ijuma commented on code in PR #18384:
URL: https://github.com/apache/kafka/pull/18384#discussion_r1902251279
##########
core/src/main/scala/kafka/server/DynamicBrokerConfig.scala:
##########
@@ -1068,8 +1059,7 @@ class DynamicListenerConfig(server: KafkaBroker) extends
BrokerReconfigurable wi
listenersToMap(newConfig.effectiveAdvertisedBrokerListeners))) {
verifyListenerRegistrationAlterationSupported()
server match {
- case kafkaServer: KafkaServer =>
kafkaServer.kafkaController.updateBrokerInfo(kafkaServer.createBrokerInfo)
- case _ => throw new RuntimeException("Unable to handle
non-kafkaServer")
+ case _ => throw new RuntimeException("Unable to handle reconfigure")
Review Comment:
If we always throw this exception, we probably don't need to do
`verifyListenerRegistrationAlterationSupported` before that and we may be able
to delete that method if it's not used anywhere else.
##########
core/src/main/scala/kafka/Kafka.scala:
##########
@@ -64,11 +64,7 @@ object Kafka extends Logging {
private def buildServer(props: Properties): Server = {
val config = KafkaConfig.fromProps(props, doLog = false)
if (config.requiresZookeeper) {
- new KafkaServer(
- config,
- Time.SYSTEM,
- threadNamePrefix = None
- )
+ throw new RuntimeException("ZooKeeper is not supported")
Review Comment:
Can we not simply remove this class? It's not a public class and I don't
think anything references it besides the relevant shell script.
##########
core/src/main/scala/kafka/server/KafkaBroker.scala:
##########
@@ -69,6 +69,8 @@ object KafkaBroker {
* you do change it, be sure to make it match that regex or the system tests
will fail.
*/
val STARTED_MESSAGE = "Kafka Server started"
+
+ val MIN_INCREMENTAL_FETCH_SESSION_EVICTION_MS: Long = 120000
Review Comment:
It's a bit odd that we're not following the Scala convention here. Since
we're rewriting this stuff in Java anyway, maybe it's ok.
##########
core/src/main/scala/kafka/server/DynamicBrokerConfig.scala:
##########
@@ -1068,8 +1059,7 @@ class DynamicListenerConfig(server: KafkaBroker) extends
BrokerReconfigurable wi
listenersToMap(newConfig.effectiveAdvertisedBrokerListeners))) {
verifyListenerRegistrationAlterationSupported()
server match {
- case kafkaServer: KafkaServer =>
kafkaServer.kafkaController.updateBrokerInfo(kafkaServer.createBrokerInfo)
- case _ => throw new RuntimeException("Unable to handle
non-kafkaServer")
+ case _ => throw new RuntimeException("Unable to handle reconfigure")
Review Comment:
Also, we probably should include more context: it looks like we don't allow
dynamic reconfiguration of listener registrations.
@cmccabe @jsancio Is this documented as part of the zk to kraft migration?
--
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]
