cmccabe commented on a change in pull request #10008:
URL: https://github.com/apache/kafka/pull/10008#discussion_r568009986
##########
File path: core/src/main/scala/kafka/server/KafkaServer.scala
##########
@@ -318,8 +318,9 @@ class KafkaServer(
/* start group coordinator */
// Hardcode Time.SYSTEM for now as some Streams tests fail otherwise,
it would be good to fix the underlying issue
- groupCoordinator = GroupCoordinator(config, zkClient, replicaManager,
Time.SYSTEM, metrics)
- groupCoordinator.startup()
+ groupCoordinator = GroupCoordinator(config, replicaManager,
Time.SYSTEM, metrics)
+
groupCoordinator.startup(zkClient.getTopicPartitionCount(Topic.GROUP_METADATA_TOPIC_NAME).
Review comment:
> What's the advantage of having a coordinator that is not started?
This is a pattern that we are using for a lot of components in the kip-500
branch. It reflects the fact that in the kip-500 world, we have a "before
metadata is available" state and an "after metadata is available" state. The
advantage is that it makes this state explicit rather than being implicit.
When the state is implicit mistakes can easily happen-- for example, we don't
want someone to modify the code to ask for some piece of metadata that is not
yet available. The original author of the code may have "just known" that this
was not allowed, but it's better to formalize it so that people don't make
mistakes.
I think the callback approach is really clunky and tends to obfuscate the
code. What's going on here is really that the number of partitions gets
initialized once. Having a complex callback chain just obscures that and makes
it tough to read.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
