[GitHub] [kafka] chia7712 commented on a change in pull request #9284: KAFKA-10479 Throw exception if users try to update configs of existen…
chia7712 commented on a change in pull request #9284: URL: https://github.com/apache/kafka/pull/9284#discussion_r496340632 ## File path: docs/upgrade.html ## @@ -27,6 +27,14 @@ Notable changes in 2 default.api.timeout.ms, and Kafka Streams' new task.timeout.ms parameters instead. Note that parameter retry.backoff.ms is not impacted by this change. +Altering non-reconfigurable configs of existent listeners causes InvalidRequestException. +By contrast, the previous behavior would have caused the updated configuration to be persisted, but it wouldn't +take effect until the broker was restarted. This change breaks behavior compatibility but the old behavior is not Review comment: I had deleted the sentence about behavior compatibility :) 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
[GitHub] [kafka] chia7712 commented on a change in pull request #9284: KAFKA-10479 Throw exception if users try to update configs of existen…
chia7712 commented on a change in pull request #9284: URL: https://github.com/apache/kafka/pull/9284#discussion_r496013137 ## File path: docs/upgrade.html ## @@ -27,6 +27,14 @@ Notable changes in 2 default.api.timeout.ms, and Kafka Streams' new task.timeout.ms parameters instead. Note that parameter retry.backoff.ms is not impacted by this change. +Altering non-reconfigurable configs of existent listeners causes InvalidRequestException. +By contrast, the previous behavior would have caused the updated configuration to be persisted, but it wouldn't +take effect until the broker was restarted. This change breaks behavior compatibility but the old behavior is not +worth keeping since that behavior is unintended. see +https://github.com/apache/kafka/pull/9284;>KAFKA-10479 for more discussion. +see DynamicBrokerConfig.DynamicSecurityConfigs and SocketServer.ListenerReconfigurableConfigs +for reconfigurable configs of existent listeners. + Review comment: > I think we should keep the comment, I was suggesting we tweak the description. got it. 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
[GitHub] [kafka] chia7712 commented on a change in pull request #9284: KAFKA-10479 Throw exception if users try to update configs of existen…
chia7712 commented on a change in pull request #9284: URL: https://github.com/apache/kafka/pull/9284#discussion_r495992169 ## File path: docs/upgrade.html ## @@ -27,6 +27,14 @@ Notable changes in 2 default.api.timeout.ms, and Kafka Streams' new task.timeout.ms parameters instead. Note that parameter retry.backoff.ms is not impacted by this change. +Altering non-reconfigurable configs of existent listeners causes InvalidRequestException. +By contrast, the previous behavior would have caused the updated configuration to be persisted, but it wouldn't +take effect until the broker was restarted. This change breaks behavior compatibility but the old behavior is not +worth keeping since that behavior is unintended. see +https://github.com/apache/kafka/pull/9284;>KAFKA-10479 for more discussion. +see DynamicBrokerConfig.DynamicSecurityConfigs and SocketServer.ListenerReconfigurableConfigs +for reconfigurable configs of existent listeners. + Review comment: > The previous behavior was a bug as it failed to report an error even though it did not change the config dynamically. ok. it makes sense. I will remove this comment from ```docs/upgrade.html``` 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
[GitHub] [kafka] chia7712 commented on a change in pull request #9284: KAFKA-10479 Throw exception if users try to update configs of existen…
chia7712 commented on a change in pull request #9284:
URL: https://github.com/apache/kafka/pull/9284#discussion_r495983768
##
File path: core/src/test/scala/unit/kafka/server/DynamicBrokerConfigTest.scala
##
@@ -327,10 +327,12 @@ class DynamicBrokerConfigTest {
EasyMock.replay(kafkaServer)
props.put(KafkaConfig.ListenersProp,
"PLAINTEXT://hostname:9092,SASL_PLAINTEXT://hostname:9093")
-val newConfig = KafkaConfig(props)
+new
DynamicListenerConfig(kafkaServer).validateReconfiguration(KafkaConfig(props))
+// it is illegal to update configs of existent listeners
+props.put("listener.name.plaintext.you.should.not.pass", "failure")
Review comment:
you are right. I will revise the comment.
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
[GitHub] [kafka] chia7712 commented on a change in pull request #9284: KAFKA-10479 Throw exception if users try to update configs of existen…
chia7712 commented on a change in pull request #9284: URL: https://github.com/apache/kafka/pull/9284#discussion_r495983283 ## File path: docs/upgrade.html ## @@ -27,6 +27,14 @@ Notable changes in 2 default.api.timeout.ms, and Kafka Streams' new task.timeout.ms parameters instead. Note that parameter retry.backoff.ms is not impacted by this change. +Altering non-reconfigurable configs of existent listeners causes InvalidRequestException. +By contrast, the previous behavior would have caused the updated configuration to be persisted, but it wouldn't +take effect until the broker was restarted. This change breaks behavior compatibility but the old behavior is not +worth keeping since that behavior is unintended. see +https://github.com/apache/kafka/pull/9284;>KAFKA-10479 for more discussion. +see DynamicBrokerConfig.DynamicSecurityConfigs and SocketServer.ListenerReconfigurableConfigs +for reconfigurable configs of existent listeners. + Review comment: > Does it really break compatibility? It seems like the previous behavior wasn't working as intended anyway. It seems to me the APIs behavior is changed so the compatibility is broken. For example, the code which is used to change non-reconfigurable configs gets exception now so users have to write something to handle "new" exception. Of course, that case should be very rare so mentioning this in docs/upgrade.html is enough to this incompatible change. 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
