Re: [PR] KAFKA-20380: controller.quorum.voters should act as advertised.listeners if the latter is not defined [kafka]

["jsancio (via GitHub)" <gi...@apache.org>]

jsancio commented on code in PR #21918:
URL: https://github.com/apache/kafka/pull/21918#discussion_r3183401840


##########
core/src/main/scala/kafka/server/KafkaConfig.scala:
##########
@@ -360,21 +451,28 @@ class KafkaConfig private(doLog: Boolean, val props: util.Map[_, _])
     } else {
       Seq.empty
     }
+    val controllerQuorumVotersAddress = QuorumConfig.parseVoterConnections(quorumConfig.voters).asScala.get(nodeId())

Review Comment:
   Can we perform this parsing only when it is needed? For example, let move it to line 469, after the if statement with the 3 predicates.



##########
core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala:
##########
@@ -349,17 +349,33 @@ class KafkaConfigTest {
   }
 
   @Test
-  def testEffectAdvertiseControllerListenerForControllerWithoutAdvertisement(): Unit = {
+  def testEffectAdvertiseControllerListenerForControllerWithoutAdvertisementAndVotersConfig(): Unit = {
     val props = new Properties()
     props.setProperty(KRaftConfigs.PROCESS_ROLES_CONFIG, "controller")
-    props.setProperty(SocketServerConfigs.LISTENERS_CONFIG, "CONTROLLER://localhost:9093")
     props.setProperty(KRaftConfigs.NODE_ID_CONFIG, "2")
-    props.setProperty(QuorumConfig.QUORUM_VOTERS_CONFIG, "2@localhost:9093")
     props.setProperty(KRaftConfigs.CONTROLLER_LISTENER_NAMES_CONFIG, "CONTROLLER")
+    props.setProperty(QuorumConfig.QUORUM_VOTERS_CONFIG, "2@lb1.example.com:9092")
+    props.setProperty(SocketServerConfigs.LISTENERS_CONFIG, "CONTROLLER://:9093")
 
     val config = KafkaConfig.fromProps(props)
     assertEquals(
-      Seq(new Endpoint("CONTROLLER", SecurityProtocol.PLAINTEXT, "localhost", 9093)),
+      Seq(new Endpoint("CONTROLLER", SecurityProtocol.PLAINTEXT, "lb1.example.com", 9092)),
+      config.effectiveAdvertisedControllerListeners
+    )
+  }
+
+  @Test
+  def testEffectAdvertiseControllerListenerForControllerWithoutAdvertisementOrVotersConfig(): Unit = {

Review Comment:
   Thanks for the tests!



-- 
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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org