Skip to content

Commit 3adc8a6

Browse files
authored
persist: improve ApplyMergeResult specificity (#34207)
In the move to incremental compaction we lost some specificity in the ApplyMergeResult metric since everything is applied via `perform_subset_replacement`. Instead of unconditionally returning `ApplyMergeResult::AppliedSubset` we check if the range we are replacing covers the whole batch, and if so return `AppliedExact` instead. This should return our metric fidelity to their earlier levels. <!-- Describe the contents of the PR briefly but completely. If you write detailed commit messages, it is acceptable to copy/paste them here, or write "see commit messages for details." If there is only one commit in the PR, GitHub will have already added its commit message above. --> ### Motivation <!-- Which of the following best describes the motivation behind this PR? * This PR fixes a recognized bug. [Ensure issue is linked somewhere.] * This PR adds a known-desirable feature. [Ensure issue is linked somewhere.] * This PR fixes a previously unreported bug. [Describe the bug in detail, as if you were filing a bug report.] * This PR adds a feature that has not yet been specified. [Write a brief specification for the feature, including justification for its inclusion in Materialize, as if you were writing the original feature specification.] * This PR refactors existing code. [Describe what was wrong with the existing code, if it is not obvious.] --> ### Tips for reviewer <!-- Leave some tips for your reviewer, like: * The diff is much smaller if viewed with whitespace hidden. * [Some function/module/file] deserves extra attention. * [Some function/module/file] is pure code movement and only needs a skim. Delete this section if no tips. --> ### Checklist - [ ] This PR has adequate test coverage / QA involvement has been duly considered. ([trigger-ci for additional test/nightly runs](https://trigger-ci.dev.materialize.com/)) - [ ] This PR has an associated up-to-date [design doc](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/README.md), is a design doc ([template](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/00000000_template.md)), or is sufficiently small to not require a design. <!-- Reference the design in the description. --> - [ ] If this PR evolves [an existing `$T ⇔ Proto$T` mapping](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/command-and-response-binary-encoding.md) (possibly in a backwards-incompatible way), then it is tagged with a `T-proto` label. - [ ] If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label ([example](MaterializeInc/cloud#5021)). <!-- Ask in #team-cloud on Slack if you need help preparing the cloud PR. --> - [ ] If this PR includes major [user-facing behavior changes](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/guide-changes.md#what-changes-require-a-release-note), I have pinged the relevant PM to schedule a changelog post.
1 parent d89aeb9 commit 3adc8a6

File tree

1 file changed

+60
-1
lines changed

1 file changed

+60
-1
lines changed

src/persist-client/src/internal/trace.rs

Lines changed: 60 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1449,6 +1449,12 @@ impl<T: Timestamp + Lattice + Codec64> SpineBatch<T> {
14491449
});
14501450
new_parts.extend_from_slice(&parts[range.end..]);
14511451

1452+
let res = if range.len() == parts.len() {
1453+
ApplyMergeResult::AppliedExact
1454+
} else {
1455+
ApplyMergeResult::AppliedSubset
1456+
};
1457+
14521458
let new_spine_batch = SpineBatch {
14531459
id: *id,
14541460
desc: desc.to_owned(),
@@ -1462,7 +1468,7 @@ impl<T: Timestamp + Lattice + Codec64> SpineBatch<T> {
14621468
}
14631469

14641470
*self = new_spine_batch;
1465-
ApplyMergeResult::AppliedSubset
1471+
res
14661472
}
14671473
}
14681474

@@ -2574,4 +2580,57 @@ pub(crate) mod tests {
25742580
prop_assert!(new_batch.run_meta.len() == batch.run_meta.len() - runs.len() + to_replace.run_meta.len());
25752581
});
25762582
}
2583+
2584+
#[mz_ore::test]
2585+
fn test_perform_subset_replacement() {
2586+
let batch1 = crate::internal::state::tests::hollow::<u64>(0, 10, &["a"], 10);
2587+
let batch2 = crate::internal::state::tests::hollow::<u64>(10, 20, &["b"], 10);
2588+
let batch3 = crate::internal::state::tests::hollow::<u64>(20, 30, &["c"], 10);
2589+
2590+
let id_batch1 = IdHollowBatch {
2591+
id: SpineId(0, 1),
2592+
batch: Arc::new(batch1.clone()),
2593+
};
2594+
let id_batch2 = IdHollowBatch {
2595+
id: SpineId(1, 2),
2596+
batch: Arc::new(batch2.clone()),
2597+
};
2598+
let id_batch3 = IdHollowBatch {
2599+
id: SpineId(2, 3),
2600+
batch: Arc::new(batch3.clone()),
2601+
};
2602+
2603+
let spine_batch = SpineBatch {
2604+
id: SpineId(0, 3),
2605+
desc: Description::new(
2606+
Antichain::from_elem(0),
2607+
Antichain::from_elem(30),
2608+
Antichain::from_elem(0),
2609+
),
2610+
parts: vec![id_batch1, id_batch2, id_batch3],
2611+
active_compaction: None,
2612+
len: 30,
2613+
};
2614+
2615+
let res_exact = crate::internal::state::tests::hollow::<u64>(0, 30, &["d"], 30);
2616+
let mut sb_exact = spine_batch.clone();
2617+
let result = sb_exact.perform_subset_replacement(&res_exact, SpineId(0, 3), 0..3, None);
2618+
assert!(matches!(result, ApplyMergeResult::AppliedExact));
2619+
assert_eq!(sb_exact.parts.len(), 1);
2620+
assert_eq!(sb_exact.len(), 30);
2621+
2622+
let res_subset = crate::internal::state::tests::hollow::<u64>(0, 20, &["e"], 20);
2623+
let mut sb_subset = spine_batch.clone();
2624+
let result = sb_subset.perform_subset_replacement(&res_subset, SpineId(0, 2), 0..2, None);
2625+
assert!(matches!(result, ApplyMergeResult::AppliedSubset));
2626+
assert_eq!(sb_subset.parts.len(), 2); // One new part + one old part
2627+
assert_eq!(sb_subset.len(), 30);
2628+
2629+
let res_too_big = crate::internal::state::tests::hollow::<u64>(0, 30, &["f"], 31);
2630+
let mut sb_too_big = spine_batch.clone();
2631+
let result = sb_too_big.perform_subset_replacement(&res_too_big, SpineId(0, 3), 0..3, None);
2632+
assert!(matches!(result, ApplyMergeResult::NotAppliedTooManyUpdates));
2633+
assert_eq!(sb_too_big.parts.len(), 3);
2634+
assert_eq!(sb_too_big.len(), 30);
2635+
}
25772636
}

0 commit comments

Comments
 (0)