POC: run group coordinator state machine on request handler pool

[artemlivshits]

This is a PoC (not tested and probably needs some corner cases addressed) for running group coordinator on the request handler pool instead of dedicated thread pool.

The main new piece of code is NonBlockingSynchronizer that is an async equivalent of locking -- it makes sure that async tasks with a given key don't run concurrently. The actual work is scheduled back on the request thread pool using KafkaRequestHandler.wrap primitive that was first introduced for KIP-890 work.

[dajac]

Thanks for the PR. I believe that isolating the workload from the produce/fetch workload (running in the api handlers) is actually a good thing but we would need to measure to compare both approaches.

Would it be possible to implement a new CoordinatorEventProcessor for this instead of hijacking the runtime? We made it pluggable for this reason. We wanted to have some flexibility with the threading model.

[artemlivshits]

True, using a separate thread pool has its value. Generally, a separate thread pool is used when the patterns are significantly different, e.g. threads that use only compute vs. threads that also do sync IO, or maybe threads that run at a higher priority or etc. In a fully async system, the number of worker threads is just a function of number of CPUs, proliferation of threads is suboptimal because more cycles are spent on context switching. It seems to me that group coordinator is fairly aligned with what other requests do -- it does sync IO on its threads pretty much the same as other requests do. Offset commit is a part of the core data path (especially for EoS). We could say that other requests technically are not on core data path, but I'm not sure what would justify a separate thread pool for those.

Would it be possible to implement a new CoordinatorEventProcessor

It should be possible (given that it has just one method), but I'm not sure if this would help with loosening the coupling. E.g. the event is still coupled to the accumulator, has partition granularity, etc. Thread pool and synchronizer, on the other hand, are general utilities and are not coupled to each other. This example illustrates implementing coordinator event processor on request handler threads but it's actually much more powerful: we can as easily change the thread pool without changing the synchronization or change synchronization (e.g. make it group granularity, not partition granularity) without changing the which thread pool to use.

[dajac]

I am not sure to fully understand your point. You cannot change the granularity without changing how events are created in the first place. Moreover, CoordinatorEventProcessor is not right to any accumulators. It is an interface allowing you to implement a strategy to execute the events. You can use a thread pool, run them in the executor thread, or however you want. So this seems to be the right way to change how events are executed while keeping it pluggable, isn’t it? It would be great if we could respect the abstractions in place.

My point is that I would be happy to merge another CoordinatorEventProcessor implementation because we can then choose the one that we want to use, based on benchmarks. If you are not interested in doing this, I would suggest to close this PR because it won’t go anywhere.

[github-actions[bot]]

This PR is being marked as stale since it has not had any activity in 90 days. If you would like to keep this PR alive, please ask a committer for review. If the PR has merge conflicts, please update it with the latest from trunk (or appropriate release branch)

If this PR is no longer valid or desired, please feel free to close it. If no activity occurs in the next 30 days, it will be automatically closed.