Harden upsert compaction CRC mismatch handling - #19084
Open
Vamsi-klu wants to merge 1 commit into
Open
Conversation
Compaction tasks could look successful while skipping work, or fail hard on reload races and data-CRC-equivalent segments. Align checks with crcMatches, meter skip/fail reasons, and retry transient server bitmap CRC mismatches.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #19084 +/- ##
============================================
+ Coverage 65.47% 65.51% +0.04%
- Complexity 1421 1438 +17
============================================
Files 3426 3426
Lines 217315 217378 +63
Branches 34509 34519 +10
============================================
+ Hits 142283 142419 +136
+ Misses 63513 63443 -70
+ Partials 11519 11516 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Author
|
Ready for review — all required CI checks are green on this PR. Issue: #13491 Could the following folks take a look when convenient? Formal GitHub "Request review" is unavailable from a fork contributor account on Thank you! |
Jackie-Jiang
added
enhancement
Contributor
There was a problem hiding this comment.
Pull request overview
Hardens upsert compaction task execution by aligning CRC validation across ZK/deepstore/server (including data-CRC fallback), improving observability for “skip vs fail” outcomes, and adding a bounded retry to mitigate transient server reload races during validDocIds bitmap fetch.
Changes:
- Add deepstore CRC validation using the same segment/data CRC matching semantics as server-side matching, plus clearer skip/fail progress messages and meters.
- Add bounded retry for validDocIds fetch on transient failures, with metering for terminal failure classification.
- Expand unit coverage for new CRC match/mismatch branches, skip metering, and parsing/matching helpers.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/test/java/org/apache/pinot/plugin/minion/tasks/upsertcompaction/UpsertCompactionTaskExecutorTest.java | Adds targeted unit tests for deepstore CRC validation, bitmap-fetch retry behavior, and skip metering. |
| pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/test/java/org/apache/pinot/plugin/minion/tasks/MinionTaskUtilsTest.java | Extends tests for new string CRC matching overload and parseCrc behavior. |
| pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/test/java/org/apache/pinot/plugin/minion/tasks/BaseSingleSegmentConversionExecutorTest.java | Adds coverage for ZK CRC-change skip path metering. |
| pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/upsertcompaction/UpsertCompactionTaskExecutor.java | Implements data-CRC-aware deepstore CRC validation, bounded bitmap-fetch retry, and clearer skip messaging + meters. |
| pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/MinionTaskUtils.java | Promotes CRC matching helper(s) to support deepstore/executor alignment; adds string overload + parseCrc utility. |
| pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks/src/main/java/org/apache/pinot/plugin/minion/tasks/BaseSingleSegmentConversionExecutor.java | Improves “ZK CRC changed” skip observability via progress message + meter. |
| pinot-common/src/main/java/org/apache/pinot/common/metrics/MinionMeter.java | Adds new Minion meters for upsert-compaction CRC/skip/failure observability paths. |
Comment on lines
+177
to
+178
| RoaringBitmap bitmap = new RoaringBitmap(); | ||
| bitmap.add(0, 1, 2); |
Comment on lines
+78
to
+101
|
|
||
| @BeforeMethod | ||
| public void setUp() | ||
| throws Exception { | ||
| _minionMetrics = mock(MinionMetrics.class); | ||
| // Force the process-global singleton so BaseTaskExecutor picks up the mock. | ||
| // register() only wins when current is NOOP; tests may run after other classes registered. | ||
| java.lang.reflect.Field field = MinionMetrics.class.getDeclaredField("MINION_METRICS_INSTANCE"); | ||
| field.setAccessible(true); | ||
| @SuppressWarnings("unchecked") | ||
| java.util.concurrent.atomic.AtomicReference<MinionMetrics> ref = | ||
| (java.util.concurrent.atomic.AtomicReference<MinionMetrics>) field.get(null); | ||
| ref.set(_minionMetrics); | ||
|
|
||
| _eventObserver = MinionTaskTestUtils.getMinionProgressObserver(); | ||
| _tempDir = new File(FileUtils.getTempDirectory(), "UpsertCompactionTaskExecutorTest-" + System.nanoTime()); | ||
| Assert.assertTrue(_tempDir.mkdirs()); | ||
| } | ||
|
|
||
| @AfterMethod | ||
| public void tearDown() | ||
| throws Exception { | ||
| FileUtils.deleteDirectory(_tempDir); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Upsert compaction must apply a server
validDocIdsbitmap only to a matching on-disk segment body. CRC mismatches across ZK / deepstore / replica servers caused tasks to skip work while looking healthy, or to fail opaquely on reload races. Ops could not tell “benign skip” from “hard failure,” and space reclaim stalled.Much of the original “WARN then COMPLETED” path has hardened over time; this PR finishes observability, data-CRC alignment, and targeted retry without redesigning segment commit.
Impact
crcMatches).ignoreCrcMismatchdefaults; still refuses to compact mismatched bytes+bitmap pairs.How
MinionTaskUtils.crcMatches(segment CRC or data CRC).Test plan
UpsertCompactionTaskExecutorTest— CRC match/mismatch, ignore flag, empty bitmap, null bitmap fail.MinionTaskUtilsTest/BaseSingleSegmentConversionExecutorTestextensions for skip metering paths../mvnw -pl pinot-plugins/pinot-minion-tasks/pinot-minion-builtin-tasks -am -Dtest=UpsertCompactionTaskExecutorTest,MinionTaskUtilsTest,BaseSingleSegmentConversionExecutorTest -Dsurefire.failIfNoSpecifiedTests=false testRelated
fixes: #13491
Reviewers
Suggested: tibrewalpratik17 (issue author)
Was generative AI tooling used to co-author this PR?
Generated-by: Grok Build (xAI)