Re: [PR] KAFKA-16515: Fix the ZK Metadata cache confusion between brokers and controllers [kafka]
cmccabe commented on PR #16006: URL: https://github.com/apache/kafka/pull/16006#issuecomment-2133754024 @jolshan : Sorry, there was a typo in the original commit that led to a variable getting shadowed. I think it happened during rebasing on trunk. This should be fixed now. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16515: Fix the ZK Metadata cache confusion between brokers and controllers [kafka]
jolshan commented on PR #16006: URL: https://github.com/apache/kafka/pull/16006#issuecomment-2131316535 https://github.com/apache/kafka/pull/16082 to revert. If folks can find a fix quickly that also works. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16515: Fix the ZK Metadata cache confusion between brokers and controllers [kafka]
jolshan commented on PR #16006: URL: https://github.com/apache/kafka/pull/16006#issuecomment-2131315366 Are we sure the failures are not related? It seems there are consistent ZKMigrationIntegrationTest failures on trunk since this change. Reverting the change fixes the issue. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16515: Fix the ZK Metadata cache confusion between brokers and controllers [kafka]
cmccabe commented on PR #16006: URL: https://github.com/apache/kafka/pull/16006#issuecomment-2130026904 failures not related. committed, thanks -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16515: Fix the ZK Metadata cache confusion between brokers and controllers [kafka]
cmccabe closed pull request #16006: KAFKA-16515: Fix the ZK Metadata cache confusion between brokers and controllers URL: https://github.com/apache/kafka/pull/16006 -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16515: Fix the ZK Metadata cache confusion between brokers and controllers [kafka]
cmccabe commented on PR #16006: URL: https://github.com/apache/kafka/pull/16006#issuecomment-2123537218 Tests should be fixed now. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16515: Fix the ZK Metadata cache confusion between brokers and controllers [kafka]
cmccabe commented on code in PR #16006:
URL: https://github.com/apache/kafka/pull/16006#discussion_r1608921813
##
core/src/main/scala/kafka/server/metadata/ZkMetadataCache.scala:
##
@@ -350,11 +347,7 @@ class ZkMetadataCache(
override def getAliveBrokerNode(brokerId: Int, listenerName: ListenerName):
Option[Node] = {
val snapshot = metadataSnapshot
-brokerId match {
- case id if
snapshot.controllerId.filter(_.isInstanceOf[KRaftCachedControllerId]).exists(_.id
== id) =>
-kraftControllerNodeMap.get(id)
- case _ =>
snapshot.aliveBrokers.get(brokerId).flatMap(_.getNode(listenerName))
-}
+snapshot.aliveBrokers.get(brokerId).flatMap(_.getNode(listenerName))
Review Comment:
I recall why we did this now (I didn't originally)...
During ZK migration, we wanted a way to make the controller node provider
work with both ZK and KRaft controllers.
The correct way to do this is just to consult `RaftManager` for the endpoint
when we're dealing with a KRaft controller (which we do know)
But at the time, we "hacked" it by adding the controllers to the cache as
"brokers" for some functions but not others (ew)
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16515: Fix the ZK Metadata cache confusion between brokers and controllers [kafka]
jsancio commented on code in PR #16006:
URL: https://github.com/apache/kafka/pull/16006#discussion_r1608902290
##
core/src/main/scala/kafka/server/metadata/ZkMetadataCache.scala:
##
@@ -350,11 +347,7 @@ class ZkMetadataCache(
override def getAliveBrokerNode(brokerId: Int, listenerName: ListenerName):
Option[Node] = {
val snapshot = metadataSnapshot
-brokerId match {
- case id if
snapshot.controllerId.filter(_.isInstanceOf[KRaftCachedControllerId]).exists(_.id
== id) =>
-kraftControllerNodeMap.get(id)
- case _ =>
snapshot.aliveBrokers.get(brokerId).flatMap(_.getNode(listenerName))
-}
+snapshot.aliveBrokers.get(brokerId).flatMap(_.getNode(listenerName))
Review Comment:
Interesting. Do you know why this was added in the first place? @akhileshchg
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[PR] KAFKA-16515: Fix the ZK Metadata cache confusion between brokers and controllers [kafka]
cmccabe opened a new pull request, #16006: URL: https://github.com/apache/kafka/pull/16006 ZkMetadataCache could theoretically return KRaft controller information from a call to ZkMetadataCache.getAliveBrokerNode, which doesn't make sense. KRaft controllers are not part of the set of brokers. In practice, this wasn't a concern since all the use-cases for ZkMetadataCache.getAliveBrokerNode center around finding coordinators, which will never be kraft controllers anyway. Still, cleaning up this code reduces confusion and is helpful for removing places where static controller configurations are used, as part of KIP-853. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
