Gate preprocess() on needPreprocess() in cold-download and consuming segment callers - #19394
Merged
Jackie-Jiang merged 9 commits intoSep 3, 2026
Merged
Conversation
deepthi912
marked this pull request as draft
August 29, 2026 01:07
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19394 +/- ##
============================================
- Coverage 67.63% 67.62% -0.02%
Complexity 1430 1430
============================================
Files 3487 3488 +1
Lines 224326 224392 +66
Branches 35408 35424 +16
============================================
+ Hits 151725 151745 +20
- Misses 60573 60609 +36
- Partials 12028 12038 +10
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:
|
deepthi912
marked this pull request as ready for review
August 31, 2026 06:27
deepthi912
force-pushed
the
honor-skip-preprocess-in-load
branch
5 times, most recently
from
August 31, 2026 23:44
576a4e7 to
1249e39
Compare
deepthi912
changed the title
deepthi912
changed the title
…overload
The 4-arg load(File, IndexLoadingConfig, throttler, SegmentZKMetadata) overload
hard-coded needPreprocess=true, so every caller of it defeated the
skipSegmentPreprocess flag. That covered four production call sites:
- BaseTableDataManager#downloadAndLoadSegment (also reached by
#replaceSegmentIfCrcMismatch)
- BaseTableDataManager reload tail (line 1204)
- RealtimeTableDataManager#downloadAndReplaceConsumingSegment
- RealtimeTableDataManager#replaceConsumingSegment
Fix the overload itself instead of each call site: replace the hard-coded
true with !indexLoadingConfig.isSkipSegmentPreprocess(). All four callers
now inherit the correct behaviour, and no future caller of this overload
can silently reintroduce the bug.
preprocess()'s contract is untouched; needPreprocess() remains the gate,
but the overload — which is itself a caller of preprocess() — now
consults the flag before delegating.
Route the four production cold-download callsites (downloadAndLoadSegment, the reload-after-CRC-mismatch branch, downloadAndReplaceConsumingSegment, replaceConsumingSegment) through ImmutableSegmentLoader#needPreprocess before invoking load(...). Mirrors the warm-load pattern already used in BaseTableDataManager, so preprocess() is skipped when the segment is already consistent (pauseless-realtime replica downloading a tar the committer preprocessed) or when skipSegmentPreprocess is set. Revert the 4-arg load(File, ILC, throttler, zkMetadata) overload to its original delegation now that callers gate the check themselves. Add an initSegmentDirectory(File, ...) overload for callers that hold the index directory directly, and a computeNeedPreprocess helper that opens the SegmentDirectory, invokes needPreprocess(), and closes.
TierBasedSegmentDirectoryLoader.load() physically moves srcDir to a tier-specific destDir when the target tier differs from the source. The check was invoking it through initSegmentDirectory, so the segment was moved away before the subsequent ImmutableSegmentLoader.load ran on the original indexDir. Switch to DefaultSegmentDirectoryLoader for the check-only path — it opens a plain SegmentLocalFSDirectory and never moves.
deepthi912
force-pushed
the
honor-skip-preprocess-in-load
branch
from
September 2, 2026 17:36
34dc6fc to
55867dd
Compare
Jackie-Jiang
reviewed
Sep 2, 2026
Jackie-Jiang
left a comment
Jackie-Jiang
left a comment
Contributor
There was a problem hiding this comment.
I think we should simply integrate need pre-process check into ImmutableSegmentLoader.load() after checking the needPreprocess flag. The flag means whether the caller asks the loader to perform preprocess; the loader decides whethner preprocess is needed
Per Jackie's review: the boolean parameter to load(File, ILC, boolean,
...) is now the caller's opt-in signal ("please consider preprocess"),
and the loader decides whether preprocess is actually needed via
needPreprocess(SegmentDirectory, ILC). Preprocess runs only when the
caller opts in and the loader agrees work is pending.
The check uses the default (non-tier-aware) SegmentDirectoryLoader so it
never physically moves the segment across tiers; the configured loader
is still used for the final open.
Reverts the caller-side computeNeedPreprocess helper and the
initSegmentDirectory(File, ...) overload — callers pass true directly.
Restore ImmutableSegmentLoader.load(File, ILC, throttler, zkMetadata) so production callers in BaseTableDataManager and RealtimeTableDataManager don't need to change. Since the smart check lives inside the 5-arg load(...) that this overload delegates to, existing callers get the new behavior for free. Minimal-touch diff per Jackie's review.
Collaborator
Author
|
Done |
Jackie-Jiang
approved these changes
Sep 3, 2026
xiangfu0
added a commit
to pinot-contrib/pinot-docs
that referenced
this pull request
Sep 3, 2026
Documents the corrected skipSegmentPreprocess behavior from apache/pinot#19394. - covers cold downloads and replacement paths - explains needPreprocess gating and faster availability - preserves tier-override guidance and cautions Validation: `scripts/validate-docs.py --changed-only=... --strict`; `git diff --check` Co-authored-by: Xiang Fu <xiangfu@Xiang-mac-mtv-2.local>
Contributor
|
Documentation follow-up: pinot-contrib/pinot-docs#1029 (merged). |
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
5 participants
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.
Summary
ImmutableSegmentLoader#needPreprocessshort-circuits whenskipSegmentPreprocessis set, and after #19391 also honors tier-scoped overrides. Callers on the warm-load and reload paths route through it and observe the flag correctly.But the cold-download path —
BaseTableDataManager#downloadAndLoadSegmentand#replaceSegmentIfCrcMismatch— invokes the 4-argImmutableSegmentLoader#loadoverload that hard-codesneedPreprocess=trueand callspreprocessdirectly. Preprocess itself never consulted the flag, so every handler ran on cold load.This PR moves the flag check into
preprocessitself, so every caller behaves consistently.Why this matters
Fast ingest. When a freshly built segment first lands on a server,
tryLoadExistingSegmenthas no local copy to reuse, sodownloadAndLoadSegmentruns — and preprocess with it. ForskipSegmentPreprocess=truetables that trade initial index coverage for fast ingest, this defeats the flag on the exact path that matters. Really helps for OFFLINE tables who don't need to think of query performance immediatelyWith this fix, fresh segments come
ONLINEimmediately after the tar downloads. Handler work can then be deferred to reload / tier migration / manual triggers instead of blocking ingest.