-
Notifications
You must be signed in to change notification settings - Fork 15.2k
KAFKA-18533: Remove KafkaConfig zookeeper related logic #18547
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
1908217
dbc87c4
1ee34b4
7bc0660
c9b8a34
57c6334
5ac12a3
5b687b3
c81d9bb
719905a
90e50e7
9644491
b6992fc
909aabc
221fde7
fe5f209
f8a8192
46a9460
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,7 +33,6 @@ trait Server { | |
| object Server { | ||
| val MetricsPrefix: String = "kafka.server" | ||
| val ClusterIdLabel: String = "kafka.cluster.id" | ||
| val BrokerIdLabel: String = "kafka.broker.id" | ||
| val NodeIdLabel: String = "kafka.node.id" | ||
|
|
||
| def initializeMetrics( | ||
|
|
@@ -69,13 +68,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) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please remove
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. addressed it :) |
||
| } | ||
|
|
||
| contextLabels.put(NodeIdLabel, config.nodeId.toString) | ||
| contextLabels.putAll(config.originalsWithPrefix(CommonClientConfigs.METRICS_CONTEXT_PREFIX)) | ||
| new KafkaMetricsContext(MetricsPrefix, contextLabels) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,6 +49,17 @@ import org.junit.jupiter.api.function.Executable | |
| import scala.jdk.CollectionConverters._ | ||
|
|
||
| class KafkaConfigTest { | ||
|
|
||
| def createDefaultConfig(): Properties = { | ||
| val props = new Properties() | ||
| props.setProperty(KRaftConfigs.PROCESS_ROLES_CONFIG, "broker,controller") | ||
| props.setProperty(KRaftConfigs.CONTROLLER_LISTENER_NAMES_CONFIG, "CONTROLLER") | ||
| props.setProperty(KRaftConfigs.NODE_ID_CONFIG, "1") | ||
| props.setProperty(SocketServerConfigs.LISTENERS_CONFIG, "PLAINTEXT://localhost:0,CONTROLLER://localhost:5000") | ||
| props.setProperty(QuorumConfig.QUORUM_VOTERS_CONFIG, "2@localhost:5000") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this config is invalid. It uses combined mode so the id should be aligned with @mingyen066 Could you please file a minor to fix it? @m1a2st is busy today
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, I've create a minor fix PR |
||
| props.setProperty(SocketServerConfigs.LISTENER_SECURITY_PROTOCOL_MAP_CONFIG, "PLAINTEXT:PLAINTEXT,CONTROLLER:SASL_SSL") | ||
| props | ||
| } | ||
|
|
||
| @Test | ||
| def testLogRetentionTimeHoursProvided(): Unit = { | ||
|
|
@@ -547,9 +558,7 @@ class KafkaConfigTest { | |
|
|
||
| @Test | ||
| def testListenerNameMissingFromListenerSecurityProtocolMap(): Unit = { | ||
| val props = new Properties() | ||
| props.setProperty(ServerConfigs.BROKER_ID_CONFIG, "1") | ||
| props.setProperty(ZkConfigs.ZK_CONNECT_CONFIG, "localhost:2181") | ||
| val props = createDefaultConfig() | ||
|
|
||
| props.setProperty(SocketServerConfigs.LISTENERS_CONFIG, "SSL://localhost:9091,REPLICATION://localhost:9092") | ||
| props.setProperty(ReplicationConfigs.INTER_BROKER_LISTENER_NAME_CONFIG, "SSL") | ||
|
|
@@ -558,9 +567,7 @@ class KafkaConfigTest { | |
|
|
||
| @Test | ||
| def testInterBrokerListenerNameMissingFromListenerSecurityProtocolMap(): Unit = { | ||
| val props = new Properties() | ||
| props.setProperty(ServerConfigs.BROKER_ID_CONFIG, "1") | ||
| props.setProperty(ZkConfigs.ZK_CONNECT_CONFIG, "localhost:2181") | ||
| val props = createDefaultConfig() | ||
|
|
||
| props.setProperty(SocketServerConfigs.LISTENERS_CONFIG, "SSL://localhost:9091") | ||
| props.setProperty(ReplicationConfigs.INTER_BROKER_LISTENER_NAME_CONFIG, "REPLICATION") | ||
|
|
@@ -569,9 +576,7 @@ class KafkaConfigTest { | |
|
|
||
| @Test | ||
| def testInterBrokerListenerNameAndSecurityProtocolSet(): Unit = { | ||
| val props = new Properties() | ||
| props.setProperty(ServerConfigs.BROKER_ID_CONFIG, "1") | ||
| props.setProperty(ZkConfigs.ZK_CONNECT_CONFIG, "localhost:2181") | ||
| val props = createDefaultConfig() | ||
|
|
||
| props.setProperty(SocketServerConfigs.LISTENERS_CONFIG, "SSL://localhost:9091") | ||
| props.setProperty(ReplicationConfigs.INTER_BROKER_LISTENER_NAME_CONFIG, "SSL") | ||
|
|
@@ -794,11 +799,6 @@ class KafkaConfigTest { | |
|
|
||
| KafkaConfig.configNames.foreach { name => | ||
| name match { | ||
| case ZkConfigs.ZK_CONNECT_CONFIG => // ignore string | ||
| case ZkConfigs.ZK_SESSION_TIMEOUT_MS_CONFIG => assertPropertyInvalid(baseProperties, name, "not_a_number") | ||
| case ZkConfigs.ZK_CONNECTION_TIMEOUT_MS_CONFIG => assertPropertyInvalid(baseProperties, name, "not_a_number") | ||
| case ZkConfigs.ZK_ENABLE_SECURE_ACLS_CONFIG => assertPropertyInvalid(baseProperties, name, "not_a_boolean") | ||
| case ZkConfigs.ZK_MAX_IN_FLIGHT_REQUESTS_CONFIG => assertPropertyInvalid(baseProperties, name, "not_a_number", "0") | ||
| case ZkConfigs.ZK_SSL_CLIENT_ENABLE_CONFIG => assertPropertyInvalid(baseProperties, name, "not_a_boolean") | ||
| case ZkConfigs.ZK_CLIENT_CNXN_SOCKET_CONFIG => //ignore string | ||
| case ZkConfigs.ZK_SSL_KEY_STORE_LOCATION_CONFIG => //ignore string | ||
|
|
@@ -1181,7 +1181,6 @@ class KafkaConfigTest { | |
| defaults.setProperty(QuorumConfig.QUORUM_BOOTSTRAP_SERVERS_CONFIG, "CONTROLLER://localhost:9092") | ||
| defaults.setProperty(KRaftConfigs.CONTROLLER_LISTENER_NAMES_CONFIG, "CONTROLLER") | ||
| // For ZkConnectionTimeoutMs | ||
| defaults.setProperty(ZkConfigs.ZK_SESSION_TIMEOUT_MS_CONFIG, "1234") | ||
| defaults.setProperty(ServerConfigs.BROKER_ID_GENERATION_ENABLE_CONFIG, "false") | ||
| defaults.setProperty(ServerConfigs.RESERVED_BROKER_MAX_ID_CONFIG, "1") | ||
| defaults.setProperty(ServerConfigs.BROKER_ID_CONFIG, "1") | ||
|
|
@@ -1198,7 +1197,6 @@ class KafkaConfigTest { | |
| defaults.setProperty(MetricConfigs.METRIC_RECORDING_LEVEL_CONFIG, Sensor.RecordingLevel.DEBUG.toString) | ||
|
|
||
| val config = KafkaConfig.fromProps(defaults) | ||
| assertEquals(1234, config.zkConnectionTimeoutMs) | ||
| assertEquals(false, config.brokerIdGenerationEnable) | ||
| assertEquals(1, config.maxReservedBrokerId) | ||
| assertEquals(1, config.brokerId) | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also remove them from
ZkConfigs?