dkranchii commented on code in PR #19925:
URL: https://github.com/apache/pinot/pull/19925#discussion_r4318057422
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/segment/index/JsonIndexTest.java:
##########
@@ -1104,6 +1104,37 @@ public void
testSkipInvalidJsonDisabledContinueOnErrorFalse() throws Exception {
createIndex(true, jsonIndexConfig, records);
}
+ /// The off-heap creator opens the posting list stream in the constructor,
but seal() is never reached when add()
+ /// fails. close() must still release the stream and clean up the temp
directory, and must stay idempotent so that
+ /// the normal seal()-then-close() path keeps working.
+ @Test
+ public void testOffHeapCreatorCloseWithoutSeal()
+ throws Exception {
+ String columnName = "offHeapUnsealed";
Review Comment:
@xiangfu0 Good catch. My checkout still had target/classes from a build
predating this change (no close() override), so I ran the test logic against
both the un-fixed and fixed class — against the un-fixed one every assertion
passed, exactly as you described.
The test now probes the stream directly. Reflection is needed because the
field is private, and a flush() is needed because the buffering layer accepts
writes without touching the file descriptor, so writing alone would pass on a
closed stream. The new assertFalse(isPostingListStreamOpen(creator)) fails
against the un-fixed class and passes against the fixed one. I also added an
assertTrue positive control before close() so the post-close assertion can't
pass just because the probe is broken.
One caveat: the sealed-creator assertion is not a regression guard, since
seal() closes the stream either way. It only catches a double-close regression
on the happy path.
Took both minor notes too. Reworded the comment that overstated the JDK
guarantee, and added a constructor try/catch so a failure opening the stream
cleans up the temp dir — it calls super.close() because the override
dereferences the not-yet-assigned stream, and suppresses cleanup failures so
they can't mask the original exception. That path has its own test, which also
fails pre-fix and passes post-fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
