showuon commented on code in PR #12405:
URL: https://github.com/apache/kafka/pull/12405#discussion_r922794782
##########
core/src/main/scala/kafka/controller/ControllerContext.scala:
##########
@@ -328,8 +328,9 @@ class ControllerContext {
}
def queueTopicDeletion(topics: Set[String]): Unit = {
+ val newlyDeletedTopics = topics.diff(topicsToBeDeleted)
topicsToBeDeleted ++= topics
Review Comment:
Since we've calculated the diff, we can directly add the
`newlyDeletedTopics` to avoid unnecessary duplication, right?
##########
core/src/main/scala/kafka/controller/ControllerContext.scala:
##########
@@ -328,8 +328,9 @@ class ControllerContext {
}
def queueTopicDeletion(topics: Set[String]): Unit = {
+ val newlyDeletedTopics = topics.diff(topicsToBeDeleted)
Review Comment:
1. Could we add a comment here to mention why we need `diff` here?
2. I don't think the parameter name `topics` is clear enough. Could we
update it? Ex: topics -> `newTopicsToBeDeleted` or
`topicToBeAddedIntoDeletionList` or something else?
##########
core/src/main/scala/kafka/controller/ControllerContext.scala:
##########
@@ -328,8 +328,9 @@ class ControllerContext {
}
def queueTopicDeletion(topics: Set[String]): Unit = {
+ val newlyDeletedTopics = topics.diff(topicsToBeDeleted)
Review Comment:
One question: After this change, `topic-A` in your example in JIRA will be
only entered into `cleanPreferredReplicaImbalanceMetric` once, right? So, what
if when the first time entering `cleanPreferredReplicaImbalanceMetric`, it
haven't run topic-A's deletion procedure to mark topic-A's all partitions as
Offline (Leader = -1) here:
https://github.com/apache/kafka/blob/3.2.0/core/src/main/scala/kafka/controller/ReplicaStateMachine.scala#L368
? I mean, since we only decrement `preferredReplicaImbalanceCount` if topic-A
has NoLeader and `!hasPreferredLeader(replicaAssignment, leadershipInfo)`
evaluates to true, right? So, what if the topic-A haven't reached this
condition and we entering `cleanPreferredReplicaImbalanceMetric`? Will that
cause some partitions count should be decremented but they are not? Or I should
say, will that case happen?
--
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
