rondagostino commented on a change in pull request #11503:
URL: https://github.com/apache/kafka/pull/11503#discussion_r765940228
##########
File path: core/src/main/scala/kafka/server/KafkaConfig.scala
##########
@@ -1959,10 +1969,28 @@ class KafkaConfig private(doLog: Boolean, val props:
java.util.Map[_, _], dynami
}
}
- def listenerSecurityProtocolMap: Map[ListenerName, SecurityProtocol] = {
- getMap(KafkaConfig.ListenerSecurityProtocolMapProp,
getString(KafkaConfig.ListenerSecurityProtocolMapProp))
+ def effectiveListenerSecurityProtocolMap: Map[ListenerName,
SecurityProtocol] = {
Review comment:
> validations in DynamicListenerConfig make sense in the context of
these changes?
We do have this code, which means we currently don't have to worry about
this for KRaft:
https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/server/DynamicBrokerConfig.scala#L909-L911
However, we will support this eventually, so let's put that aside for the
moment.
We do have these checks, which continue to make sense:
```
if (immutableListenerConfigs(newConfig, listenerName.configPrefix) !=
immutableListenerConfigs(oldConfig, listenerName.configPrefix))
throw new ConfigException(s"Configs cannot be updated dynamically
for existing listener $listenerName, " +
"restart broker or create a new listener for update")
if (oldConfig.effectiveListenerSecurityProtocolMap(listenerName) !=
newConfig.effectiveListenerSecurityProtocolMap(listenerName))
throw new ConfigException(s"Security protocol cannot be updated for
existing listener $listenerName")
```
One thing that could occur is that we add a new SSL listener that is the
first one that is non-PLAINTEXT and then all of a sudden we have removed any
PLAINTEXT default values in the listener security protocol map, which could
cause the new config to fail. But that's fine. And this would never happen in
a non-toy cluster since those generally have at least one secured/non-PLAINTEXT
listener anyway.
Another possibility is that we might impact existing controller listeners,
but that impact would have to be checked for when we implement this dynamic
reconfiguration path for KRaft.
My take on this is that the existing changes are okay in this context.
--
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]
