https server will not be impacted by the MaxConcurrentStreams settin… #14081

YANGJINJUE · 2022-05-30T09:29:57Z

https server will not be impacted by the MaxConcurrentStreams settings of grpc opts

ahrtr · 2022-05-30T09:52:22Z

server/embed/serve.go

@@ -17,6 +17,7 @@ package embed
 import (
 	"context"
 	"fmt"
+	"golang.org/x/net/http2"


move this to the following block (starting from line 29)

server/embed/serve.go

ptabor · 2022-05-30T12:26:32Z

Is it possible to add a repro tests for the problem, to make sure it will not regress and that logic is consistent between http & https. please ?

nic-chen · 2022-05-31T12:04:48Z

I think we could flow recommendations of @ptabor and @ahrtr to make the pull request done:

let's expose this as an explicit flag that applies to both http, https & grpc (with default of 100).
let's have a test that shows that it works (e.g. when the flag is 5, the 6 independent watch connections cannot be established)
let's have some measurement on memory impact on number of connections. E.g Prometheus graphs showing a load-test of growing number of connections versus RSS memory usage, to be able to comment on the flag the memory expections for the high values of the flag.

About the default value, I think it is better to use math.MaxUint32 which have been using so far. Otherwise, users may have problems with more than 100 connections (eg users from Apache APISIX).

Next, we can take a benchmark and give recommended value based on hardware.

What do you think? @ptabor @ahrtr @serathius

nic-chen · 2022-05-31T12:05:09Z

@YANGJINJUE

Would you mind if we update your pull request?

We hope to move forward to resolve this issue as soon as possible.

ahrtr · 2022-05-31T12:25:39Z

About the default value, I think it is better to use math.MaxUint32 which have been using so far. Otherwise, users may have problems with more than 100 connections (eg users from Apache APISIX).

Next, we can take a benchmark and give recommended value based on hardware.

What do you think? @ptabor @ahrtr @serathius

I think It's OK to set the default value as math.MaxUint32. Revisiting all the maximum limitations (including this one) is in my to do list.

YANGJINJUE · 2022-06-01T05:50:43Z

@YANGJINJUE

Would you mind if we update your pull request?

We hope to move forward to resolve this issue as soon as possible.

ok

nic-chen · 2022-06-02T00:54:50Z

@YANGJINJUE
Would you mind if we update your pull request?
We hope to move forward to resolve this issue as soon as possible.

ok

Oh, I can't push my code.
Could you invite me to your repo please? @YANGJINJUE
Thanks!

YANGJINJUE · 2022-06-02T02:25:01Z

You already have permission

codecov-commenter · 2022-06-02T04:12:13Z

Codecov Report

Merging #14081 (5276968) into main (08f4c34) will decrease coverage by 0.33%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##             main   #14081      +/-   ##
==========================================
- Coverage   75.29%   74.95%   -0.34%     
==========================================
  Files         452      453       +1     
  Lines       36791    36803      +12     
==========================================
- Hits        27701    27585     -116     
- Misses       7364     7461      +97     
- Partials     1726     1757      +31

Flag	Coverage Δ
all	`74.95% <71.42%> (-0.34%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
server/config/config.go	`79.76% <ø> (ø)`
server/embed/serve.go	`60.79% <50.00%> (-0.40%)`	⬇️
server/etcdmain/grpc_proxy.go	`58.89% <60.00%> (+0.01%)`	⬆️
pkg/flags/int.go	`76.47% <76.47%> (ø)`
server/embed/config.go	`73.64% <100.00%> (+0.06%)`	⬆️
server/embed/etcd.go	`74.95% <100.00%> (+0.04%)`	⬆️
server/etcdmain/config.go	`86.23% <100.00%> (+0.11%)`	⬆️
server/etcdserver/api/v3rpc/grpc.go	`91.42% <100.00%> (ø)`
server/proxy/grpcproxy/register.go	`69.76% <0.00%> (-9.31%)`	⬇️
server/etcdserver/api/v3lock/lock.go	`61.53% <0.00%> (-8.47%)`	⬇️
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 08f4c34...5276968. Read the comment docs.

nic-chen · 2022-06-05T01:49:47Z

hi @ptabor @ahrtr @serathius
Please help to review at your convenience. Thanks!

ahrtr · 2022-06-07T22:39:56Z

Please fix the test failures.

Signed-off-by: nic-chen <chenjunxu6@gmail.com>

ahrtr · 2022-06-22T05:31:54Z

any update? Please let me know if you need my help on this PR.

nic-chen · 2022-06-22T13:16:09Z

hi @ahrtr
Sorry, I haven't found a solution to the test failure, and I can't pass it stably;
I have been out on business for this month, and basically have no time to solve this problem for now;
Would be grateful if you could help take a look! Thank you very much!

ahrtr · 2022-06-23T22:28:07Z

hi @ahrtr Sorry, I haven't found a solution to the test failure, and I can't pass it stably; I have been out on business for this month, and basically have no time to solve this problem for now; Would be grateful if you could help take a look! Thank you very much!

Thanks for letting me know. I will get this one resolved next week or the week after the next.

There is no update on the original PR (see below) for more then 2 weeks. So I continue to work on the PR. The first step is to rebase the PR. ``` etcd-io#14081 ``` Signed-off-by: nic-chen <chenjunxu6@gmail.com> Signed-off-by: Benjamin Wang <wachao@vmware.com>

@ahrtr

There is no update on the original PR (see below) for more then 2 weeks. So Benjamin(@ahrtr) continue to work on the PR. The first step is to rebase the PR, because there are lots of conflicts with the main branch. ``` etcd-io#14081 ``` Signed-off-by: nic-chen <chenjunxu6@gmail.com> Signed-off-by: Benjamin Wang <wachao@vmware.com>

@ahrtr

There is no update on the original PR (see below) for more then 2 weeks. So Benjamin(@ahrtr) continues to work on the PR. The first step is to rebase the PR, because there are lots of conflicts with the main branch. ``` etcd-io#14081 ``` Signed-off-by: nic-chen <chenjunxu6@gmail.com> Signed-off-by: Benjamin Wang <wachao@vmware.com>

@ahrtr

There is no update on the original PR (see below) for more then 2 weeks. So Benjamin(@ahrtr) continues to work on the PR. The first step is to rebase the PR, because there are lots of conflicts with the main branch. The change to go.mod and go.sum reverted, because they are not needed. The e2e test cases are also reverted, because they are not correct. ``` etcd-io#14081 ``` Signed-off-by: nic-chen <chenjunxu6@gmail.com> Signed-off-by: Benjamin Wang <wachao@vmware.com>

@ahrtr

There is no update on the original PR (see below) for more then 2 weeks. So Benjamin(@ahrtr) continues to work on the PR. The first step is to rebase the PR, because there are lots of conflicts with the main branch. The change to go.mod and go.sum reverted, because they are not needed. The e2e test cases are also reverted, because they are not correct. ``` etcd-io#14081 ``` Signed-off-by: nic-chen <chenjunxu6@gmail.com> Signed-off-by: Benjamin Wang <wachao@vmware.com>

@ahrtr

There is no update on the original PR (see below) for more then 2 weeks. So Benjamin(@ahrtr) continues to work on the PR. The first step is to rebase the PR, because there are lots of conflicts with the main branch. The change to go.mod and go.sum reverted, because they are not needed. The e2e test cases are also reverted, because they are not correct. ``` etcd-io#14081 ``` Signed-off-by: nic-chen <chenjunxu6@gmail.com> Signed-off-by: Benjamin Wang <wachao@vmware.com>

ahrtr · 2022-07-13T07:26:47Z

#14169 was merged, so closing this one.

YANGJINJUE mentioned this pull request May 30, 2022

fix https-> ConcurrentMaxStream bug #13985

Closed

ahrtr reviewed May 30, 2022

View reviewed changes

nic-chen force-pushed the main branch from 757efdf to c725d59 Compare June 2, 2022 03:41

nic-chen force-pushed the main branch from 6ce005b to 0bd38a5 Compare June 5, 2022 01:46

nic-chen force-pushed the main branch from e436bf9 to a5e8301 Compare June 5, 2022 06:23

nic-chen force-pushed the main branch 7 times, most recently from 3ce69ff to 0424af8 Compare June 14, 2022 00:03

feat: support custom grpc.MaxConcurrentStreams

5e1dcfd

Signed-off-by: nic-chen <chenjunxu6@gmail.com>

nic-chen force-pushed the main branch from 0424af8 to 5e1dcfd Compare June 14, 2022 02:21

ahrtr mentioned this pull request Jun 21, 2022

Etcd unexpected request time consumption with HTTPs v3 API #14077

Closed

ahrtr mentioned this pull request Jun 22, 2022

Plans for v3.5.5 release #14138

Closed

16 tasks

ahrtr mentioned this pull request Jun 23, 2022

embed: add GRPCAdditionalServerOptions config #14066

Merged

ahrtr mentioned this pull request Jun 27, 2022

Support configuring MaxConcurrentStreams for http2 #14169

Merged

ahrtr closed this Jul 13, 2022

iori0758 mentioned this pull request Sep 8, 2022

help request: upgrade to 2.14.0 after the route matching error, adjust the priority also did not solve, back to 2.13.1 on the normal apache/apisix#7136

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

https server will not be impacted by the MaxConcurrentStreams settin… #14081

https server will not be impacted by the MaxConcurrentStreams settin… #14081

YANGJINJUE commented May 30, 2022

ahrtr May 30, 2022

ptabor commented May 30, 2022

nic-chen commented May 31, 2022

nic-chen commented May 31, 2022

ahrtr commented May 31, 2022

YANGJINJUE commented Jun 1, 2022

nic-chen commented Jun 2, 2022

YANGJINJUE commented Jun 2, 2022

codecov-commenter commented Jun 2, 2022 •

edited

Loading

nic-chen commented Jun 5, 2022

ahrtr commented Jun 7, 2022

ahrtr commented Jun 22, 2022

nic-chen commented Jun 22, 2022

ahrtr commented Jun 23, 2022

ahrtr commented Jul 13, 2022 •

edited

Loading

https server will not be impacted by the MaxConcurrentStreams settin… #14081

https server will not be impacted by the MaxConcurrentStreams settin… #14081

Conversation

YANGJINJUE commented May 30, 2022

ahrtr May 30, 2022

Choose a reason for hiding this comment

ptabor commented May 30, 2022

nic-chen commented May 31, 2022

nic-chen commented May 31, 2022

ahrtr commented May 31, 2022

YANGJINJUE commented Jun 1, 2022

nic-chen commented Jun 2, 2022

YANGJINJUE commented Jun 2, 2022

codecov-commenter commented Jun 2, 2022 • edited Loading

Codecov Report

nic-chen commented Jun 5, 2022

ahrtr commented Jun 7, 2022

ahrtr commented Jun 22, 2022

nic-chen commented Jun 22, 2022

ahrtr commented Jun 23, 2022

ahrtr commented Jul 13, 2022 • edited Loading

codecov-commenter commented Jun 2, 2022 •

edited

Loading

ahrtr commented Jul 13, 2022 •

edited

Loading