iemejia commented on PR #8511:
URL: https://github.com/apache/hadoop/pull/8511#issuecomment-6480743114
Thanks @pan3793 for the detailed review. I've rebased onto trunk and pushed
the fixes addressing your comments:
1. **Pooled-buffer reuse:** `RawLocalFileSystem` now limits each buffer to
the requested range length before submitting the read. The regression test
reuses the same pool for the smaller `[7680, 512)` tail range and verifies that
an oversized checksum buffer is reused without reading past EOF.
2. **Exceptional completion:** checksum cleanup now uses `whenComplete`, so
buffers are released after all dependent verifications finish, including when
verification or a data read fails. Failed underlying reads retain
responsibility for their own buffers, avoiding double releases. Successfully
read data buffers that cannot be returned because verification or a checksum
read failed are also released; successful sibling buffers remain caller-owned.
3. **Release contract:** I've proposed updates to both `PositionedReadable`
and `fsdatainputstream.md` distinguishing internal buffers from caller-visible
data buffers. Returned buffers and their backing storage must remain
caller-owned; internal checksum buffers can be released once no operation needs
them, regardless of success or failure.
@steveloughran, could you confirm whether this distinction matches the
intended contract, including cleanup of internal reads after synchronous setup
fails?
4. **Tests:** the tests now honor heap/direct parameterization, replace
sleeps with latches, and assert exact release counts. Coverage includes pooled
tail reads, checksum corruption, truncated checksum files, data-read failures,
and preservation of successful sibling buffers. Identity tracking checks for
leaks and duplicate releases.
The follow-up review also uncovered synchronous submission failures that
could leave buffers unreleased and futures pending. Those paths are now handled
and covered by deterministic tests, including rejected partial-read
resubmissions and inline callbacks.
After the rebase, all **183 cases across the five affected test classes pass
with JDK 17**. I've also updated the PR description to reflect the final
implementation and test coverage.
--
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]
