Do not rely on assert for null/empty check on realtime commit temp folder - #19261
Open
dkranchii wants to merge 1 commit into
Open
Do not rely on assert for null/empty check on realtime commit temp folder#19261dkranchii wants to merge 1 commit into
dkranchii wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #19261 +/- ##
============================================
- Coverage 67.11% 67.10% -0.02%
Complexity 1423 1423
============================================
Files 3457 3457
Lines 219284 219290 +6
Branches 34864 34864
============================================
- Hits 147167 147149 -18
- Misses 60346 60363 +17
- Partials 11771 11778 +7
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:
|
dkranchii
force-pushed
the
fix/realtime-segment-data-manager-assert
branch
from
August 16, 2026 02:00
66d7535 to
9c04105
Compare
…timeSegmentDataManager
`buildSegmentInternal` used `assert tempFiles != null;` before indexing into
`tempFiles[0]` to pick the built segment's temp index directory. Java `assert`
is a no-op unless the JVM is started with `-ea`, and Pinot does not run its
service processes with assertions enabled — so if `tempSegmentFolder.listFiles()`
returned `null` (I/O error, folder gone) or an empty array (racy cleanup),
production would throw `NullPointerException` or `ArrayIndexOutOfBoundsException`
from a segment-commit path instead of the typed `SegmentBuildFailureException`
the caller and Helix state machine already expect.
Extract a small `@VisibleForTesting` static helper `pickTempIndexDir(...)`
that performs the check and throws `SegmentBuildFailureException` with a clear
message. The caller in `buildSegmentInternal` continues to funnel the failure
through `reportSegmentBuildFailure` (metric + error info), matching the pattern
already used for the "metadata file missing" / "creation meta missing"
branches further down in the same method.
Add unit tests for the helper covering:
- happy path: folder with one entry returns that entry
- empty folder: throws `SegmentBuildFailureException`
- non-existent folder (`listFiles()` returns `null`): throws
`SegmentBuildFailureException`
No behavior change on the happy path. No config, SPI, or wire-format change.
Co-authored-by: Cursor <cursoragent@cursor.com>
dkranchii
force-pushed
the
fix/realtime-segment-data-manager-assert
branch
from
August 16, 2026 05:09
9c04105 to
2232c42
Compare
| File[] tempFiles = tempSegmentFolder.listFiles(); | ||
| assert tempFiles != null; | ||
| File tempIndexDir = tempFiles[0]; | ||
| File tempIndexDir; |
Contributor
There was a problem hiding this comment.
Suggest just inline the change. I feel it is more readable that way. It is okay to not have test if it is hard to add
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
3 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
RealtimeSegmentDataManager.buildSegmentInternalused a Javaassertbefore indexing intotempSegmentFolder.listFiles():Java
assertis a no-op unless the JVM is started with-ea, and Pinot's service processes do not run with assertions enabled. IflistFiles()returnednull(I/O error, folder gone) or an empty array (race with cleanup), production would raiseNullPointerException/ArrayIndexOutOfBoundsExceptioninstead of the typedSegmentBuildFailureExceptionthat the caller and the Helix state machine already handle. That leaves the realtime segment commit in an ambiguous state.Change
@VisibleForTestingstatic helperpickTempIndexDir(File)that performs the null/empty check and throwsSegmentBuildFailureException("Temp segment folder is empty or unreadable: ...").buildSegmentInternalfunnels the failure throughreportSegmentBuildFailure(msg, null)before rethrowing — matching the pattern already used a few lines down for the missing-metadata / missing-creation-meta branches.No behavior change on the happy path. No config, SPI, wire-format, or metric-name change.
Tests
Added three unit tests to
RealtimeSegmentDataManagerTest:testPickTempIndexDirReturnsFirstEntry— happy path, folder with one entry returns that entry.testPickTempIndexDirThrowsWhenFolderIsEmpty— empty folder →SegmentBuildFailureException.testPickTempIndexDirThrowsWhenFolderIsMissing— non-existent folder (listFiles()returnsnull) →SegmentBuildFailureException. This is the exact scenario the oldassertmasked.Run:
Scope
Single-file production change in
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/RealtimeSegmentDataManager.java, plus tests in the adjacent test file.Release note
none (turns a rare NPE / AIOOBE on the segment-commit path into the typed
SegmentBuildFailureExceptionthat Helix handling already anticipates)