xiangfu0 commented on code in PR #19925:
URL: https://github.com/apache/pinot/pull/19925#discussion_r4309189821
##########
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:
**MAJOR (test quality): this test does not actually verify the leak fix.**
Every assertion here passes even if the production `close()` override is
removed, so it is not a regression guard for the leaked file descriptor.
Walking it with the fix reverted:
- `assertThrows(JsonProcessingException.class, ...)` is pure
`add()`/`flatten()` behavior, independent of `close()`.
- `assertFalse(tempDir.exists())` passes because base
`BaseJsonIndexCreator.close()` calls `FileUtils.deleteDirectory(_tempDir)`.
- The second `creator.close()` passes because `deleteDirectory` on a missing
dir returns silently (idempotent).
- The sealed-creator path is untouched by the override.
Nothing asserts that `_postingListOutputStream` was closed / the FD released
— which is the exact regression this test claims to cover. Consider asserting
the stream/FD state directly (e.g. spy or wrap the output stream and verify
`close()` was invoked, or assert the underlying channel is no longer open) so
the test fails when the override is absent.
--
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]
