[fs] Delegate skipFully to hadoop's IOUtils - #9779
Draft
LuciferYang wants to merge 5 commits into
Draft
Conversation
skipFully looped while (bytes > 0) subtracting whatever in.skip returned. A blocking stream at end of file returns 0 from skip, so a small forward seek (up to 1 MiB, used by the vectored-read helpers) past a truncated or shorter-than-expected file spun the loop forever instead of failing the read with an end-of-file error. Throw EOFException when a skip call makes no progress. The same loop was replicated in HadoopFileIO and in every HadoopCompliantFileIO wrapper (azure, cosn, gs, jindo, obs, oss, s3); all copies are updated. Assisted-by: GLM-5.3
LuciferYang
marked this pull request as draft
September 13, 2026 03:07
The hand-rolled loop declared EOF on the first zero-byte skip, but InputStream.skip may return 0 without being at the end, and the issue's own reference, org.apache.hadoop.io.IOUtils.skipFully, resolves that by probing with read(). It ships in hadoop-common, which every one of these modules already depends on for FSDataInputStream, so delegating both fixes the detection and removes eight byte-identical copies of the loop. Co-Authored-By: Claude Code <noreply@anthropic.com>
Co-Authored-By: Claude Code <noreply@anthropic.com>
…loop did Bounds the stub so a looping caller fails instead of hanging the fork, pins skip-probe-skip in order with no other interaction, and fixes two comments that claimed more than any assertion checked.
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
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.
Purpose
close #9778
HadoopSeekableInputStream.seekturns a small forward gap (up toMIN_SKIP_BYTES, 1 MiB) intoskipFullyrather than a real seek, which for an object store avoids tearing down and reopening the connection. The loop it used wasand
skipreturning 0 makes no progress, so a seek past the end of a truncated or shorter-than-expected file spun forever. A hang is the worst way for this to fail: nothing to catch, nothing in the logs.The first version of this fix threw
EOFExceptionon the first zero-byte skip. That is not quite right, and it is also work already done upstream.InputStream.skipis documented to return 0 for reasons other than EOF, so a zero has to be resolved by reading, which is exactly whatorg.apache.hadoop.io.IOUtils.skipFullydoes:It ships in hadoop-common, which every module here already depends on for
FSDataInputStream, so this delegates to it. That fixes the detection and removes eight byte-identical copies of the loop at the same time: the net change is 96 lines deleted against 40 added, where the first version added 144.For the streams actually in play a zero does mean EOF (
FSInputStreamdoes not overrideskip, so it inherits the read-basedInputStream.skip, andBufferedFSInputStream.skipreturnsnunconditionally forn > 0), so this is not a live data bug on any filesystem in the tree. It matters for the wrappers, which adapt arbitrary Hadoop filesystems including ones outside this repo.Tests
HadoopFileIOSkipFullyTestcovers the three cases the loop got wrong or right by accident:skipreturns 0 and the probe reads -1, so it throwsEOFException.skipreturns 0 once and then makes progress. This is the case the first version of this fix broke.skipat all.Two details of how they assert, both of which took a mutant to get right. The first stub returns 0 once and then throws, rather than returning 0 forever: against the loop this replaces that turns an unbounded spin into a failed assertion naming it, so the test reports the bug instead of hanging the fork. A
@Timeoutcannot do that job here, since Jupiter's default thread mode isSAME_THREADandthreadModearrived in 5.9 while this build is on 5.8.1. And the transient-zero case pins the whole conversation in order —skip(4096),read(),skip(4095)— withverifyNoMoreInteractions, because each weaker form let a real accounting bug through:verify(read())alone passes a delegate that probes without crediting the probed byte, addingverify(skip(4095))still passes one that credits the probe but under-asks by one up front, and the in-order chain alone does not constrain totals, so a delegate probing twice passes it.Verified on JDK 11. Three tests pass; each fails for a reason the other two do not. Reverting
skipFullyto the loop on master fails two of them withskip was called again after returning 0; restoring the fail-fast version fails two; and the three accounting mutants above each fail exactly one assertion.skipFullyis byte-identical in all 17 hadoop-common jars from 2.2.0 to 3.4.3, so 4095 is the right number for every version this can run against, and the interaction set cannot grow without a change to that method. All eight touched modules (paimon-commonplus the seven filesystem impls) build with checkstyle and spotless enabled.