ijuma commented on code in PR #18547:
URL: https://github.com/apache/kafka/pull/18547#discussion_r1925448634
##########
core/src/main/scala/kafka/server/KafkaConfig.scala:
##########
@@ -734,7 +723,7 @@ class KafkaConfig private(doLog: Boolean, val props:
util.Map[_, _])
val listenerNames = listeners.map(_.listenerName).toSet
if (processRoles.isEmpty || processRoles.contains(ProcessRole.BrokerRole))
{
- // validations for all broker setups (i.e. ZooKeeper and KRaft
broker-only and KRaft co-located)
+ // validations for all broker setups (i.e. KRaft broker-only and KRaft
co-located)
Review Comment:
Shall we remove `KRaft`?
##########
core/src/main/scala/kafka/server/Server.scala:
##########
@@ -69,13 +69,7 @@ object Server {
): KafkaMetricsContext = {
val contextLabels = new java.util.HashMap[String, Object]
contextLabels.put(ClusterIdLabel, clusterId)
-
- if (config.usesSelfManagedQuorum) {
- contextLabels.put(NodeIdLabel, config.nodeId.toString)
- } else {
- contextLabels.put(BrokerIdLabel, config.brokerId.toString)
- }
-
+ contextLabels.put(BrokerIdLabel, config.brokerId.toString)
Review Comment:
Hmm, this seems like a bug - we may want to add a unit test if nothing
failed. We should be setting `NodeIdLabel`, not `BrokerIdLabel`. Right?
##########
core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala:
##########
@@ -4082,7 +4082,7 @@ object PlaintextAdminIntegrationTest {
new AlterConfigOp(new ConfigEntry(TopicConfig.COMPRESSION_TYPE_CONFIG,
"lz4"), OpType.SET)
))
alterConfigs.put(topicResource2, util.Arrays.asList(new AlterConfigOp(new
ConfigEntry(TopicConfig.COMPRESSION_TYPE_CONFIG, "snappy"), OpType.SET)))
- alterConfigs.put(brokerResource, util.Arrays.asList(new AlterConfigOp(new
ConfigEntry(ZkConfigs.ZK_CONNECT_CONFIG, "localhost:2181"), OpType.SET)))
+ alterConfigs.put(brokerResource, util.Arrays.asList(new AlterConfigOp(new
ConfigEntry(KRaftConfigs.NODE_ID_CONFIG, "123"), OpType.SET)))
Review Comment:
Hmm, it would be weird to update the node id, can we pick a config that
makes more sense to update? Same for other cases similar to this one.
--
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]
