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 2020/06/09 15:52:40 UTC
[GitHub] [kafka] ijuma commented on a change in pull request #8672: KAFKA-10002; Improve performances of StopReplicaRequest with large number of partitions to be deleted
ijuma commented on a change in pull request #8672:
URL: https://github.com/apache/kafka/pull/8672#discussion_r436791602
##########
File path: core/src/main/scala/kafka/log/LogManager.scala
##########
@@ -878,8 +936,7 @@ class LogManager(logDirs: Seq[File],
// Now that replica in source log directory has been successfully renamed for deletion.
// Close the log, update checkpoint files, and enqueue this log to be deleted.
sourceLog.close()
- checkpointRecoveryOffsetsAndCleanSnapshot(sourceLog.parentDirFile, ArrayBuffer.empty)
- checkpointLogStartOffsetsInDir(sourceLog.parentDirFile)
+ checkpointRecoveryAndLogStartOffsetsInDir(sourceLog.parentDirFile)
Review comment:
Well, they are tightly coupled right? The method name of `deleteSnapshotsAfterRecoveryPointCheckpoint` makes it clear that this should be called after the recovery point is checkpointed. Generally, we've had bugs whenever we left it to the callers to make the same multiple calls in sequence in multiple places.
I haven't looked at this PR in detail, so there are probably good reasons to change it. Also keep in mind https://github.com/apache/kafka/pull/7929/files that tries to improve the approach on how we handle this more generally.
##########
File path: core/src/main/scala/kafka/log/LogManager.scala
##########
@@ -878,8 +936,7 @@ class LogManager(logDirs: Seq[File],
// Now that replica in source log directory has been successfully renamed for deletion.
// Close the log, update checkpoint files, and enqueue this log to be deleted.
sourceLog.close()
- checkpointRecoveryOffsetsAndCleanSnapshot(sourceLog.parentDirFile, ArrayBuffer.empty)
- checkpointLogStartOffsetsInDir(sourceLog.parentDirFile)
+ checkpointRecoveryAndLogStartOffsetsInDir(sourceLog.parentDirFile)
Review comment:
Well, they are tightly coupled right? The method name of `deleteSnapshotsAfterRecoveryPointCheckpoint` makes it clear that this should be called after the recovery point is checkpointed. Generally, we've had bugs whenever we left it to the callers to make the same multiple calls in sequence in multiple places.
I haven't looked at this PR in detail, so there are probably good reasons to change it. Also keep in mind https://github.com/apache/kafka/pull/7929/files which tries to improve the approach on how we handle this more generally.
##########
File path: core/src/main/scala/kafka/log/LogManager.scala
##########
@@ -878,8 +936,7 @@ class LogManager(logDirs: Seq[File],
// Now that replica in source log directory has been successfully renamed for deletion.
// Close the log, update checkpoint files, and enqueue this log to be deleted.
sourceLog.close()
- checkpointRecoveryOffsetsAndCleanSnapshot(sourceLog.parentDirFile, ArrayBuffer.empty)
- checkpointLogStartOffsetsInDir(sourceLog.parentDirFile)
+ checkpointRecoveryAndLogStartOffsetsInDir(sourceLog.parentDirFile)
Review comment:
Well, they are tightly coupled right? The method name of `deleteSnapshotsAfterRecoveryPointCheckpoint` makes it clear that this should be called after the recovery point is checkpointed. Generally, we've had bugs whenever we left it to the callers to make the same multiple calls in sequence in multiple places.
I haven't looked at this PR in detail, so there are probably good reasons to change it. Also keep in mind #7929 which tries to improve the approach on how we handle this more generally.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org