Watch random scheduler #15431
Watch random scheduler #15431
Conversation
serathius
force-pushed
the
watch-random-scheduler
branch
2 times, most recently
from
89fb3c2 to
c28a160
Compare
serathius
force-pushed
the
watch-random-scheduler
branch
7 times, most recently
from
ad22de0 to
781de33
Compare
server/embed/serve.go
Outdated
| @@ -224,6 +224,7 @@ func configureHttpServer(srv *http.Server, cfg config.ServerConfig) error { | |||
| // todo (ahrtr): should we support configuring other parameters in the future as well? | |||
| return http2.ConfigureServer(srv, &http2.Server{ | |||
| MaxConcurrentStreams: cfg.MaxConcurrentStreams, | |||
| NewWriteScheduler: http2.NewRandomWriteScheduler, | |||
There was a problem hiding this comment.
Is it possible to add a flag here? By default, it still uses priority writer.
There was a problem hiding this comment.
Don't understand. http2.NewRandomWriteScheduler is different than http2.NewPriorityWriteScheduler
There was a problem hiding this comment.
Sorry for unclear comment.
For the MaxConcurrentStreams, the user can use --max-concurrent-streams to config etcd http2 server.
So I was thinking if the pr can support to use config to choose scheduler writer, as it is behavior change.
If the response size is less than max frame size (by default 1MiB), the priority writer is not too bad in low traffic scenario.
There was a problem hiding this comment.
Random is always better than Priority as we didn't set any priorities. Without priorities it works like LIFO which is the worst algorithm for grpc you could imagine.
There was a problem hiding this comment.
LIFO is completely not compatible with existence of watches (that are at the end of the handling queue).
I think it's better to avoid having too many flags that are difficult to understand.
This setting changed so many times in live-time of 3.4.x, that If any depended on the exact semantic, we would learn about it earlier.
| } | ||
|
|
||
| func continuouslyExecuteGetAll(ctx context.Context, t *testing.T, g *errgroup.Group, c *clientv3.Client) { | ||
| mux := sync.RWMutex{} |
There was a problem hiding this comment.
perhaps we can use atomic package here.
There was a problem hiding this comment.
This is test, doesn't need to be super performant. Mutexes are simpler to get right.
There was a problem hiding this comment.
OK. I was thinking that it is running in resource-limited vm. It might be useful to prevent flaky. Thanks for the reply.
There was a problem hiding this comment.
Being able to run tests on variety of hardware is important. I would not go further with performance requirements than running on free Github workers.
|
Verified no flakes on https://github.com/serathius/etcd/actions/runs/4373994355 |
server/embed/serve.go
Outdated
| @@ -224,6 +224,7 @@ func configureHttpServer(srv *http.Server, cfg config.ServerConfig) error { | |||
| // todo (ahrtr): should we support configuring other parameters in the future as well? | |||
| return http2.ConfigureServer(srv, &http2.Server{ | |||
| MaxConcurrentStreams: cfg.MaxConcurrentStreams, | |||
| NewWriteScheduler: http2.NewRandomWriteScheduler, | |||
There was a problem hiding this comment.
LIFO is completely not compatible with existence of watches (that are at the end of the handling queue).
I think it's better to avoid having too many flags that are difficult to understand.
This setting changed so many times in live-time of 3.4.x, that If any depended on the exact semantic, we would learn about it earlier.
server/embed/serve.go
Outdated
| @@ -224,6 +224,7 @@ func configureHttpServer(srv *http.Server, cfg config.ServerConfig) error { | |||
| // todo (ahrtr): should we support configuring other parameters in the future as well? | |||
| return http2.ConfigureServer(srv, &http2.Server{ | |||
| MaxConcurrentStreams: cfg.MaxConcurrentStreams, | |||
| NewWriteScheduler: http2.NewRandomWriteScheduler, | |||
There was a problem hiding this comment.
Please add comment above the flag, describing why it's critical for any reasonable performance out of etcd grpc APIs and pointing on the issue.
serathius
force-pushed
the
watch-random-scheduler
branch
from
781de33 to
62fe431
Compare
… when sharing the same connection Signed-off-by: Marek Siarkowicz <[email protected]>
…h starvation Signed-off-by: Marek Siarkowicz <[email protected]>
serathius
force-pushed
the
watch-random-scheduler
branch
from
62fe431 to
f3533f2
Compare
Address step 1 from #15402 (comment)
cc @ahrtr @ptabor