KAFKA-18545 Remove Zookeeper logic from LogManager #18592
Conversation
github-actions
bot
added
triage
| @@ -354,7 +354,7 @@ class LogManager(logDirs: Seq[File], | |||
| addStrayLog(topicPartition, log) | |||
| warn(s"Loaded stray log: $logDir") | |||
| } else if (isStray(log)) { | |||
| // Unlike Zookeeper mode, which tracks pending topic deletions under a ZNode, KRaft is unable to prevent a topic from being recreated before every replica has been deleted. | |||
| // KRaft is unable to prevent a topic from being recreated before every replica has been deleted. | |||
There was a problem hiding this comment.
I think simply deleting the zk statement is not enough, we should do a little rephrase for it.
How about like:
| // KRaft is unable to prevent a topic from being recreated before every replica has been deleted. | |
| // In KRaft, we are unable to prevent a topic from being recreated before every replica has been deleted. |
The rest of statement we could leave it as what it is.
| @@ -354,8 +354,8 @@ class LogManager(logDirs: Seq[File], | |||
| addStrayLog(topicPartition, log) | |||
| warn(s"Loaded stray log: $logDir") | |||
| } else if (isStray(log)) { | |||
| // Unlike Zookeeper mode, which tracks pending topic deletions under a ZNode, KRaft is unable to prevent a topic from being recreated before every replica has been deleted. | |||
| // A KRaft broker with an offline directory may be unable to detect it still holds a to-be-deleted replica, | |||
| // In KRaft, we are unable to prevent a topic from being recreated before every replica has been deleted. | |||
There was a problem hiding this comment.
No reason to say "In KRaft", right?
| topicId.foreach(log.assignTopicId) | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Should we change the topid id so it's not optional?
There was a problem hiding this comment.
Although this method doesn't utilize Optional's functionality internally, the calling method Partition#createLogIfNotExists uses Optional features, and Optional is still required when passing to the UnifiedLog constructor later. Therefore, I don't see a compelling reason to avoid using Optional at this point.
There was a problem hiding this comment.
Makes sense. Unrelated to this PR, but a good clean-up would be to extract the topic id creation so it's always set by the time the UnifiedLog constructor is called (the way it works now unnecessarily exposes an optional topic id even though it is never really optional with kraft).
There was a problem hiding this comment.
It a good idea, I will file a Jira to trace this issue.
There was a problem hiding this comment.
we should do this cleanup after all zk-related paths are removed from ReplicaManager
Jira: https://issues.apache.org/jira/browse/KAFKA-18545
Committer Checklist (excluded from commit message)