jsancio commented on code in PR #15478:
URL: https://github.com/apache/kafka/pull/15478#discussion_r1523951275
##########
core/src/main/scala/kafka/raft/KafkaMetadataLog.scala:
##########
@@ -404,21 +414,33 @@ final class KafkaMetadataLog private (
* all cases.
*
* For the given predicate, we are testing if the snapshot identified by the
first argument should be deleted.
+ * The predicate returns a Some with the reason if the snapshot should be
deleted and a None if the snapshot
+ * should not be deleted
*/
- private def cleanSnapshots(predicate: OffsetAndEpoch => Boolean): Boolean = {
- if (snapshots.size < 2)
+ private def cleanSnapshots(predicate: OffsetAndEpoch =>
Option[SnapshotDeletionReason]): Boolean = {
+ if (snapshots.size < 2) {
return false
+ }
var didClean = false
snapshots.keys.toSeq.sliding(2).foreach {
case Seq(snapshot: OffsetAndEpoch, nextSnapshot: OffsetAndEpoch) =>
- if (predicate(snapshot) && deleteBeforeSnapshot(nextSnapshot)) {
- didClean = true
- } else {
- return didClean
+ predicate(snapshot) match {
+ case Some(reason) =>
+ if (deleteBeforeSnapshot(nextSnapshot, reason)) {
Review Comment:
Yes. `&&` only evaluates the right argument if the left argument is `true`.
Which is what we want because we don't want to delete the snapshot if the
predicate is false.
--
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]
