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:
us...@infra.apache.org
