showuon commented on code in PR #15951:
URL: https://github.com/apache/kafka/pull/15951#discussion_r1610854305
##########
core/src/main/scala/kafka/server/ReplicaManager.scala:
##########
@@ -2114,16 +2114,12 @@ class ReplicaManager(val config: KafkaConfig,
partition.log.foreach { _ =>
val leader = BrokerEndPoint(config.brokerId, "localhost", -1)
- // Add future replica log to partition's map
- partition.createLogIfNotExists(
- isNew = false,
- isFutureReplica = true,
- offsetCheckpoints,
- topicIds(partition.topic))
-
- // pause cleaning for partitions that are being moved and start
ReplicaAlterDirThread to move
- // replica from source dir to destination dir
- logManager.abortAndPauseCleaning(topicPartition)
+ // Add future replica log to partition's map if it's not existed
Review Comment:
@chia7712 , thanks for the comment.
For this:
> In short, alterReplicaLogDirs adds alter thread [0] only if it succeeds to
create future log of partition. Maybe maybeAddLogDirFetchers should follow same
rule? Or we can add comments to say "that is fine as
replicaAlterLogDirsManager.addFetcherForPartitions will be a no-op in this case?
I chose latter option because if we only create fetcher when future log is
inexisted, it might cause potential side effect that this fetcher is removed
when leadership change, but not get added later. I've added the comment in this
commit:
https://github.com/apache/kafka/pull/15951/commits/0d78e493e484dd4f27ba6a127616d802999f22d0
. 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
