http2: transist READY client to busy when SETTING decreases the capacity#18999
http2: transist READY client to busy when SETTING decreases the capacity#18999ggreenway merged 3 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: YaoZengzeng <yaozengzeng@huawei.com>
Signed-off-by: YaoZengzeng <yaozengzeng@huawei.com>
|
/retest |
|
Retrying Azure Pipelines: |
|
@alyssawilk PTAL :) |
|
/assign @alyssawilk |
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
Great fix, thanks! Some comments on the test but the code looks solid :-)
| ActiveTestRequest r3(*this, 0, false); | ||
| CHECK_STATE(0 /*active*/, 3 /*pending*/, 4 /*capacity*/); | ||
|
|
||
| // When the connection connects, there is zero spare capacity in this pool. |
There was a problem hiding this comment.
I think you need to update comments?
Before we had max concurrent == 2 and 2 streams, so 0 capacity.
now we have max concurrent == 3 and 3 streams, so 1 capacity left?
| cluster_->http2_options_.mutable_max_concurrent_streams()->set_value(4); | ||
| ON_CALL(*cluster_, perUpstreamPreconnectRatio).WillByDefault(Return(1)); | ||
|
|
||
| // One stream results in one connection. Two streams result in two connections. |
There was a problem hiding this comment.
Also not yours, but I think this comment is from another test and should be removed here (as long as I'm looking at it)b
| completeRequest(r1); | ||
| EXPECT_EQ(pool_->owningList(Envoy::ConnectionPool::ActiveClient::State::READY).size(), 0); | ||
|
|
||
| // Close all streams, concurrency capacity goes to -1, there should be one ready client. |
There was a problem hiding this comment.
concurrency capacity goes to 1, not -1 right?
| completeRequest(r2); | ||
| completeRequest(r3); | ||
| EXPECT_EQ(pool_->owningList(Envoy::ConnectionPool::ActiveClient::State::READY).size(), 1); | ||
| CHECK_STATE(0 /*active*/, 0 /*pending*/, 1 /*capacity*/); |
There was a problem hiding this comment.
Do you mind creating more streams before doing the close? I think there's value in regression testing that close works when capacity is negative.
Signed-off-by: YaoZengzeng <yaozengzeng@huawei.com>
|
/retest |
|
Retrying Azure Pipelines: |
|
@alyssawilk updated! |
4 participants
Commit Message: http2: transist READY client to busy when SETTING decreases the capacity
Additional Description: fixes part I of #18880
Risk Level: low
Testing: unit
Docs Changes: n/a