You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/12/16 00:19:21 UTC
[GitHub] [kafka] akhileshchg opened a new pull request, #13001: KAFKA-14446: Follow up to the comments unaddressed on Zk brokers API …
akhileshchg opened a new pull request, #13001:
URL: https://github.com/apache/kafka/pull/13001
…forwarding patch
Original patch: https://github.com/apache/kafka/pull/12961/files#
--
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
[GitHub] [kafka] ijuma commented on a diff in pull request #13001: KAFKA-14446: code style improvements for broker-to-controller forwarding
Posted by GitBox <gi...@apache.org>.
ijuma commented on code in PR #13001:
URL: https://github.com/apache/kafka/pull/13001#discussion_r1051321610
##########
core/src/main/scala/kafka/server/KafkaServer.scala:
##########
@@ -703,8 +703,8 @@ class KafkaServer(
case None =>
info(s"Broker registration for controller $controllerId is not available in the metadata cache")
}
- case None =>
- info("No controller present in the metadata cache")
+ case Some(_: KRaftCachedControllerId) | None =>
Review Comment:
Hmm, looks like you replaced an exhaustive pattern matching block with a non exhaustive one? Am I missing something?
--
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
[GitHub] [kafka] mumrah commented on a diff in pull request #13001: KAFKA-14446: code style improvements for broker-to-controller forwarding
Posted by GitBox <gi...@apache.org>.
mumrah commented on code in PR #13001:
URL: https://github.com/apache/kafka/pull/13001#discussion_r1052593947
##########
core/src/main/scala/kafka/server/KafkaServer.scala:
##########
@@ -703,8 +703,8 @@ class KafkaServer(
case None =>
info(s"Broker registration for controller $controllerId is not available in the metadata cache")
}
- case None =>
- info("No controller present in the metadata cache")
+ case Some(_: KRaftCachedControllerId) | None =>
Review Comment:
The snippet in your comment ^ looks good to me @akhileshchg. In the previous PR, my comment was just about avoiding the filter + `isInstanceOf`. The `Some(controllerId: ZkCachedController)` covers that
--
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
[GitHub] [kafka] cmccabe commented on a diff in pull request #13001: KAFKA-14446: code style improvements for broker-to-controller forwarding
Posted by GitBox <gi...@apache.org>.
cmccabe commented on code in PR #13001:
URL: https://github.com/apache/kafka/pull/13001#discussion_r1050263765
##########
core/src/main/scala/kafka/server/KafkaServer.scala:
##########
@@ -703,8 +703,8 @@ class KafkaServer(
case None =>
info(s"Broker registration for controller $controllerId is not available in the metadata cache")
}
- case None =>
- info("No controller present in the metadata cache")
+ case _ =>
Review Comment:
this one confuses me. Isn't `case None` clearer than `case _` ?
--
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
[GitHub] [kafka] ijuma commented on a diff in pull request #13001: KAFKA-14446: code style improvements for broker-to-controller forwarding
Posted by GitBox <gi...@apache.org>.
ijuma commented on code in PR #13001:
URL: https://github.com/apache/kafka/pull/13001#discussion_r1052615536
##########
core/src/main/scala/kafka/server/KafkaServer.scala:
##########
@@ -703,8 +703,8 @@ class KafkaServer(
case None =>
info(s"Broker registration for controller $controllerId is not available in the metadata cache")
}
- case None =>
- info("No controller present in the metadata cache")
+ case Some(_: KRaftCachedControllerId) | None =>
Review Comment:
@akhileshchg My bad, I hadn't seen the `| None` part.
--
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
[GitHub] [kafka] akhileshchg commented on a diff in pull request #13001: KAFKA-14446: code style improvements for broker-to-controller forwarding
Posted by GitBox <gi...@apache.org>.
akhileshchg commented on code in PR #13001:
URL: https://github.com/apache/kafka/pull/13001#discussion_r1050278848
##########
core/src/main/scala/kafka/server/KafkaServer.scala:
##########
@@ -703,8 +703,8 @@ class KafkaServer(
case None =>
info(s"Broker registration for controller $controllerId is not available in the metadata cache")
}
- case None =>
- info("No controller present in the metadata cache")
+ case _ =>
Review Comment:
Let me add more explicit condition to prevent this confusion.
--
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
[GitHub] [kafka] cmccabe commented on a diff in pull request #13001: KAFKA-14446: code style improvements for broker-to-controller forwarding
Posted by GitBox <gi...@apache.org>.
cmccabe commented on code in PR #13001:
URL: https://github.com/apache/kafka/pull/13001#discussion_r1050263485
##########
core/src/main/scala/kafka/server/BrokerToControllerChannelManager.scala:
##########
@@ -290,29 +295,27 @@ case class BrokerToControllerQueueItem(
)
class BrokerToControllerRequestThread(
+ initialNetworkClient: KafkaClient,
+ var isNetworkClientForZkController: Boolean,
networkClientFactory: ControllerInformation => KafkaClient,
metadataUpdater: ManualMetadataUpdater,
controllerNodeProvider: ControllerNodeProvider,
config: KafkaConfig,
time: Time,
threadName: String,
retryTimeoutMs: Long
-) extends InterBrokerSendThread(threadName, null, Math.min(Int.MaxValue, Math.min(config.controllerSocketTimeoutMs, retryTimeoutMs)).toInt, time, isInterruptible = false) {
-
- var isZkController = false
- private def maybeResetNetworkClient(controllerInformation: ControllerInformation,
- initialize: Boolean = false): Unit = {
- if (initialize || isZkController != controllerInformation.isZkController) {
- if (!initialize) {
- debug("Controller changed to " + (if (isZkController) "zk" else "kraft") + " mode. " +
- "Resetting network client")
- }
+) extends InterBrokerSendThread(threadName, initialNetworkClient, Math.min(Int.MaxValue, Math.min(config.controllerSocketTimeoutMs, retryTimeoutMs)).toInt, time, isInterruptible = false) {
Review Comment:
can we put each of these on one line?
--
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
[GitHub] [kafka] akhileshchg commented on a diff in pull request #13001: KAFKA-14446: code style improvements for broker-to-controller forwarding
Posted by GitBox <gi...@apache.org>.
akhileshchg commented on code in PR #13001:
URL: https://github.com/apache/kafka/pull/13001#discussion_r1050276330
##########
core/src/main/scala/kafka/server/KafkaServer.scala:
##########
@@ -703,8 +703,8 @@ class KafkaServer(
case None =>
info(s"Broker registration for controller $controllerId is not available in the metadata cache")
}
- case None =>
- info("No controller present in the metadata cache")
+ case _ =>
Review Comment:
The condition covers more than None. It includes Some(controller: KRaftCachedControllerId) as well.
--
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
[GitHub] [kafka] akhileshchg commented on a diff in pull request #13001: KAFKA-14446: code style improvements for broker-to-controller forwarding
Posted by GitBox <gi...@apache.org>.
akhileshchg commented on code in PR #13001:
URL: https://github.com/apache/kafka/pull/13001#discussion_r1052660421
##########
core/src/main/scala/kafka/server/KafkaServer.scala:
##########
@@ -703,8 +703,8 @@ class KafkaServer(
case None =>
info(s"Broker registration for controller $controllerId is not available in the metadata cache")
}
- case None =>
- info("No controller present in the metadata cache")
+ case Some(_: KRaftCachedControllerId) | None =>
Review Comment:
Okay. Removed the change and reverted it to explicit match-case.
--
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
[GitHub] [kafka] cmccabe merged pull request #13001: KAFKA-14446: code style improvements for broker-to-controller forwarding
Posted by GitBox <gi...@apache.org>.
cmccabe merged PR #13001:
URL: https://github.com/apache/kafka/pull/13001
--
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
[GitHub] [kafka] akhileshchg commented on a diff in pull request #13001: KAFKA-14446: code style improvements for broker-to-controller forwarding
Posted by GitBox <gi...@apache.org>.
akhileshchg commented on code in PR #13001:
URL: https://github.com/apache/kafka/pull/13001#discussion_r1052573317
##########
core/src/main/scala/kafka/server/KafkaServer.scala:
##########
@@ -703,8 +703,8 @@ class KafkaServer(
case None =>
info(s"Broker registration for controller $controllerId is not available in the metadata cache")
}
- case None =>
- info("No controller present in the metadata cache")
+ case Some(_: KRaftCachedControllerId) | None =>
Review Comment:
getControllerId returns Option(CachedControllerId). CachedControllerId has two inherited classes: 1) ZkCachedControllerId and 2) KRaftCachedControllerId.
So we need to cover 1) None, 2) Some(KRaftCachedControllerId), and 3) Some(ZkCachedControllerId). I'm covering all these conditions. Did I miss something, or are you saying we might miss some future inheritance cases?
BTW, I had the below code before. But that seems to have made it confusing to the reviewers. Let me know if that works for you @ijuma
```
getControllerId match {
case Some(controllerId: ZkCachedController) => ...
case _ => ...
```
--
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
[GitHub] [kafka] akhileshchg commented on a diff in pull request #13001: KAFKA-14446: code style improvements for broker-to-controller forwarding
Posted by GitBox <gi...@apache.org>.
akhileshchg commented on code in PR #13001:
URL: https://github.com/apache/kafka/pull/13001#discussion_r1052655482
##########
core/src/main/scala/kafka/server/KafkaServer.scala:
##########
@@ -703,8 +703,8 @@ class KafkaServer(
case None =>
info(s"Broker registration for controller $controllerId is not available in the metadata cache")
}
- case None =>
- info("No controller present in the metadata cache")
+ case Some(_: KRaftCachedControllerId) | None =>
Review Comment:
I thought @mumrah was suggesting using `case _`. Let me revert it again then. Also, why would `case _` disable the exhaustiveness check, isn't it like `default`?
--
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
[GitHub] [kafka] ijuma commented on a diff in pull request #13001: KAFKA-14446: code style improvements for broker-to-controller forwarding
Posted by GitBox <gi...@apache.org>.
ijuma commented on code in PR #13001:
URL: https://github.com/apache/kafka/pull/13001#discussion_r1052651610
##########
core/src/main/scala/kafka/server/KafkaServer.scala:
##########
@@ -703,8 +703,8 @@ class KafkaServer(
case None =>
info(s"Broker registration for controller $controllerId is not available in the metadata cache")
}
- case None =>
- info("No controller present in the metadata cache")
+ case Some(_: KRaftCachedControllerId) | None =>
Review Comment:
I am not sure why a revert commit was added after this discussion. `case _` disables exhaustiveness checks and hence should be used sparingly.
--
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