Skip to content

Commit c3d965a

Browse files
committed
cs/manifest: Fix scrubber false positive for replaced segments
The scrub false-positive filter in process_anomalies() only checked whether a segment with the same offset range existed in the manifest. A compacted reupload produces a replacement segment at the same offset range but with a different name (different size). When GC deleted the old segment from cloud storage while the scrubber was still referencing a stale manifest, the filter kept the anomaly because the offset range still matched—even though the current segment at that range was a different (replacement) object that existed in cloud storage. Compare generate_remote_segment_name() for the manifest entry and the reported-missing segment so that replacements with the same offset range but different identity are correctly recognized as false positives. Fixes CORE-6913. Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
1 parent cb543a7 commit c3d965a

2 files changed

Lines changed: 18 additions & 24 deletions

File tree

src/v/cloud_storage/partition_manifest.cc

Lines changed: 15 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -489,18 +489,14 @@ bool partition_manifest::contains(const segment_name& name) const {
489489
return _segments.contains(maybe_key->base_offset);
490490
}
491491

492-
bool partition_manifest::segment_with_offset_range_exists(
493-
model::offset base, model::offset committed) const {
494-
if (auto iter = find(base); iter != end()) {
495-
const auto expected_committed
496-
= _segments.get_committed_offset_column().at_index(iter.index());
497-
498-
// false when committed offset doesn't match
499-
return committed == *expected_committed;
500-
} else {
501-
// base offset doesn't match any segment
492+
bool partition_manifest::segment_with_same_identity_exists(
493+
const partition_manifest::value& meta) const {
494+
auto iter = find(meta.base_offset);
495+
if (iter == end() || iter->committed_offset != meta.committed_offset) {
502496
return false;
503497
}
498+
return generate_remote_segment_name(*iter)
499+
== generate_remote_segment_name(meta);
504500
}
505501

506502
void partition_manifest::delete_replaced_segments() { _replaced.clear(); }
@@ -2768,12 +2764,10 @@ void partition_manifest::process_anomalies(
27682764
}
27692765

27702766
if (meta.committed_offset >= get_start_offset()) {
2771-
// The segment might have been missing because it was merged with
2772-
// something else. If the offset range doesn't match a segment
2773-
// exactly, discard the anomaly. Only segments from the STM
2774-
// manifest may be merged/reuploaded.
2775-
return !segment_with_offset_range_exists(
2776-
meta.base_offset, meta.committed_offset);
2767+
// The segment might have been missing because it was merged
2768+
// with something else or replaced by a compacted reupload.
2769+
// Only segments from the STM manifest may be merged/reuploaded.
2770+
return !segment_with_same_identity_exists(meta);
27772771
} else {
27782772
// Segment belongs to the archive. No reuploads are done here.
27792773
return false;
@@ -2790,10 +2784,11 @@ void partition_manifest::process_anomalies(
27902784
}
27912785

27922786
if (anomaly_meta.at.committed_offset >= get_start_offset()) {
2793-
// Similarly to the missing segment case, if the boundaries of the
2794-
// segment where the anomaly was detected changed, drop it.
2795-
return !segment_with_offset_range_exists(
2796-
anomaly_meta.at.base_offset, anomaly_meta.at.committed_offset);
2787+
// Similarly to the missing segment case, if the boundaries
2788+
// of the segment where the anomaly was detected changed or
2789+
// the segment was replaced (e.g., compacted reupload),
2790+
// drop the anomaly.
2791+
return !segment_with_same_identity_exists(anomaly_meta.at);
27972792
} else {
27982793
return false;
27992794
}

src/v/cloud_storage/partition_manifest.h

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -323,10 +323,9 @@ class partition_manifest : public base_manifest {
323323
bool contains(const key& key) const;
324324
bool contains(const segment_name& name) const;
325325

326-
/// Check if the provided offset range matches any segment in the manifest
327-
/// exactly.
328-
bool segment_with_offset_range_exists(
329-
model::offset base, model::offset committed) const;
326+
/// Check if the manifest contains a segment with the same identity
327+
/// (offset range, size, term) as the provided metadata.
328+
bool segment_with_same_identity_exists(const value& meta) const;
330329

331330
struct add_segment_meta_result {
332331
// size in bytes of the segment(s) that has been replaced by this

0 commit comments

Comments
 (0)