artemlivshits commented on code in PR #14705:
URL: https://github.com/apache/kafka/pull/14705#discussion_r1388644791
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/runtime/CoordinatorRuntime.java:
##########
@@ -659,9 +660,21 @@ public TopicPartition key() {
*/
@Override
public void run() {
+ try {
+ runAsync().get();
Review Comment:
> This is completely unrelated in my opinion as this is true for both the
old and the new coordinator.
It's true that it's a problem with the old coordinator, and we should make
the whatever minimal fixes required for the old coordinator to work (and if it
happens to work end-to-end, which I think it might, we won't need to fix it),
but that code is going away and shouldn't define the forward-looking
architecture.
As we build the new coordinator, we should build it in a way that improves
forward-looking architecture. Keeping the right abstraction is good,
coincidentally it helps with the timelines -- we can use this proposal and use
the work that already has been done instead of doing new work of bringing
implementation details into group coordinator.
Moreover, I wonder if we need yet another thread pool to handle group
coordinator logic, I think it would be good to just re-use the request handler
threads to run this functionality. This would avoid thread pools proliferation
and also reuse various useful improvements that work only on request pool
threads, e.g. RequestLocal (hopefully we'll make it into a real thread local to
be used at the point of use instead of passing the argument), various
observability things, etc. Here is a PoC that does that using
NonBlockingSynchronizer and KafkaRequestHandler.wrap
https://github.com/apache/kafka/pull/14728/commits/46acf0220434926305b343299d2780a34bf8a7de
The NonBlockingSynchronizer replaces EventAccumulator and
MultiThreadedEventProcessor (I didn't remove them to keep the change small), it
has some perf benefits e.g. in uncontended cases, the processing continues
running on the request thread instead of being rescheduled on the gc thread
pool. I can also easily implement read-write synchronization for the
NonBlockingSynchronizer (so that readers won't block each other out), e.g. to
implement non-blocking read "lock" on group when committing offsets.
It's not to say I don't like the current code, but it feels like we
re-building functionality that we already have elsewhere in Kafka and we we
could re-use the existing building blocks so that the gc focuses on group
coordination rather than managing thread pools, getting into the details of
transactional protocol, etc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
