cmccabe commented on code in PR #18949:
URL: https://github.com/apache/kafka/pull/18949#discussion_r1999661209
##########
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:
> I'm a bit confused about what this means. Where in sharedServer do we
"send out initial dynamic configurations"?
SharedServer doesn't. This happens from BrokerServer or ControllerServer.
> From a high level, I think the current implementation is doing the
equivalent in KRaft, since the KRaft layer, which reads the snapshot, on the
broker is acting like ZK (although possibly stale) to store broker configs
(correct me if I'm wrong, I also will look more into this tmrw).
If you are confident that this will handle the case that originally
motivated this JIRA then I'm OK with doing this for now. As you recall, the
case was the broker SSL keystore file being statically set to a path that no
longer existed.
> we need to load the dynamic configurations prior to sending out the
initial dynamic configurations
I'm a bit confused about what this means. Where in sharedServer do we "send
out initial dynamic configurations"?
From a high level, I think the current implementation is doing the
equivalent in KRaft, since the KRaft layer, which reads the snapshot, on the
broker is acting like ZK (although possibly stale) to store broker configs
(correct me if I'm wrong, I also will look more into this tmrw).
> The only config I see that is used in sharedServer/raftManager that is
also part of DynamicBrokerConfig#AllDynamicConfigs is the
LISTENER_SECURITY_PROTOCOL_MAP_CONFIG. This config is read when building the
KRaft network client for the broker during startup, but only the entry for the
controller listener is used. From my understanding, the only way to invalidate
this static config if it was previously valid would be to mess with
server.properties directly and then restart the broker. Is that also a case we
also want to cover with this change?
No. That configuration is static and cannot be dynamically changed.
--
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]
