linkedin / cruise-control

Cruise-control is the first of its kind to fully automate the dynamic workload rebalance and self-healing of a Kafka cluster. It provides great value to Kafka users by simplifying the operation of Kafka clusters.
https://github.com/linkedin/cruise-control/tags
BSD 2-Clause "Simplified" License
2.74k stars 585 forks source link

Use literal config name for listeners and broker.id config #2169

Closed HenryCaiHaiying closed 2 months ago

HenryCaiHaiying commented 3 months ago

Fix issue: https://github.com/linkedin/cruise-control/issues/2168

When running embedded MetricsReporter with the upcoming Kafka 3.8, hit the following exception:

[2024-06-18 16:08:06,722] ERROR [KafkaServer id=1025] Fatal error during KafkaServer startup. Prepare to shutdown (kafka.server.KafkaServer) java.lang.NoSuchMethodError: 'java.lang.String kafka.server.KafkaConfig.ListenersProp()' at com.linkedin.kafka.cruisecontrol.metricsreporter.CruiseControlMetricsReporter.getBootstrapServers(CruiseControlMetricsReporter.java:129)

The root cause is Kafka 3.8 (maybe also 3.7) refactored the core module for many configuration parameters, see this PR: apache/kafka@1b301b3?diff=split&w=0#diff-8eb3e01716508551f203b0b37c2d5f951e93cce7ffed7c00c2b33633d7c8ed23

The easiest fix is just to use hard-coded literal string 'listeners' for this config parameter, the name of this parameter is unlikely to change (otherwise it will break million's customer's server.properties). Although you can try to change the variable name reference from KafkaConfig.ListenersProp() to SocketServerConfigs.LISTENERS_CONFIG, but SocketServerConfigs is also a new class introduced in Kafka 3.8, to do that you would have to create a new CC branch migrate_to_3_8 to make the compilation work, but maintaining different branches for different kafka version is cumbersome and hard to maintain in the long run.

This also applies on another property broker.id

This PR resolves #.

mimaison commented 2 months ago

The easiest fix is just to use hard-coded literal string 'listeners' for this config parameter, the name of this parameter is unlikely to change (otherwise it will break million's customer's server.properties). Although you can try to change the variable name reference from KafkaConfig.ListenersProp() to SocketServerConfigs.LISTENERS_CONFIG, but SocketServerConfigs is also a new class introduced in Kafka 3.8, to do that you would have to create a new CC branch migrate_to_3_8 to make the compilation work, but maintaining different branches for different kafka version is cumbersome and hard to maintain in the long run.

KafkaConfig is not part of the public API, this is why its structure changed without a KIP or deprecation notice. SocketServerConfigs is not part of the public API either. So using the string literals, which are part of the public API, is in my opinion the best option to make the Cruise Control metrics reporter work with Kafka 3.8.

Cruise Control still depends on many non-public APIs but at least this quick fix addresses this immediate issue, as 3.8 will release shortly and is already being tested by several organizations.

HenryCaiHaiying commented 2 months ago

I don't see any objections, can we move forward to merge in this PR?

mimaison commented 2 months ago

@CCisGG Can you take a look? Thanks

fvaleri commented 2 months ago

+1