kevin-wu24 commented on code in PR #18949:
URL: https://github.com/apache/kafka/pull/18949#discussion_r1980228836
##########
core/src/main/scala/kafka/server/BrokerServer.scala:
##########
@@ -194,6 +194,7 @@ class BrokerServer(
val clientMetricsReceiverPlugin = new ClientMetricsReceiverPlugin()
config.dynamicConfig.initialize(Some(clientMetricsReceiverPlugin))
+ DynamicBrokerConfig.readDynamicBrokerConfigsFromSnapshot(raftManager,
config, quotaManagers)
Review Comment:
> This will be too late to address some of the use-cases we're concerned
about, right?
Could you illuminate some of these cases for me? It seems like we're doing
this early enough in `brokerServer`'s startup to me. Are we instead concerned
that bad static configs will prevent even `SharedServer#startup`, which sets up
KRaft and metadata publishers, from completing successfully? I guess I'm just
confused as to how node can even get into a state where the static configs
would crash `SharedServer#startup`, but somehow also have valid dynamic configs?
I'm a bit confused as to what you're proposing. It sounds like this loading
config key/value pairs from the snapshot should occur before we construct the
`KafkaConfig` in `SharedServer`'s initialization (i.e. before we call
`SharedServer#start` which initializes `raftManager`)? If so, that means we
can't use `raftManager` to actually perform the snapshot read, right?
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
