mumrah commented on a change in pull request #10864:
URL: https://github.com/apache/kafka/pull/10864#discussion_r662623797
##########
File path: core/src/main/scala/kafka/raft/KafkaMetadataLog.scala
##########
@@ -312,26 +315,142 @@ final class KafkaMetadataLog private (
}
}
- override def deleteBeforeSnapshot(logStartSnapshotId: OffsetAndEpoch):
Boolean = {
- val (deleted, forgottenSnapshots) = snapshots synchronized {
- latestSnapshotId().asScala match {
- case Some(snapshotId) if (snapshots.contains(logStartSnapshotId) &&
- startOffset < logStartSnapshotId.offset &&
- logStartSnapshotId.offset <= snapshotId.offset &&
- log.maybeIncrementLogStartOffset(logStartSnapshotId.offset,
SnapshotGenerated)) =>
+ /**
+ * Delete a snapshot, advance the log start offset, and clean old log
segments. This will only happen if the
+ * following invariants all hold true:
+ *
+ * <li>This is not the latest snapshot (i.e., another snapshot proceeds this
one)</li>
+ * <li>The offset of the next snapshot is greater than the log start
offset</li>
+ * <li>The log can be advanced to the offset of the next snapshot</li>
+ *
+ * This method is not thread safe and assumes a lock on the snapshots
collection is held
+ */
+ private[raft] def deleteSnapshot(snapshotId: OffsetAndEpoch, nextSnapshotId:
OffsetAndEpoch): Boolean = {
Review comment:
I think it's mostly a side effect of how this PR evolved.
Edit:
I can revert most of this method to be like it was originally. Since we'll
be calling it for one snapshot at a time starting with the oldest, i think the
behavior will be the same.
Are you thinking of other use cases for `deleteBeforeSnapshot`, like
truncation maybe?
--
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]
