Skip to content

Aliyun, Dell: Handle EOF in OSS and ECS InputStream read() variants - #17965

Open
damansingh1313 wants to merge 1 commit into
apache:mainfrom
damansingh1313:fix-eof-handling-oss-ecs-input-streams
Open

Aliyun, Dell: Handle EOF in OSS and ECS InputStream read() variants#17965
damansingh1313 wants to merge 1 commit into
apache:mainfrom
damansingh1313:fix-eof-handling-oss-ecs-input-streams

Conversation

@damansingh1313

Copy link
Copy Markdown
Contributor

Why is this change needed?

OSSInputStream (aliyun) and EcsSeekableInputStream (dell) advance pos (and next, in OSS's case) and increment the readBytes/readOperations metric counters before checking whether the underlying stream has reached EOF (-1). When a read call actually returns -1:

  • The tracked position is advanced by one byte (read()) or decremented by one byte (read(byte[], int, int), since -1 is added to pos/next), desyncing the stream's internal position bookkeeping from the real position in the object.
  • The readBytes counter is incremented by -1 in the buffered-read path, producing a nonsensical negative byte count in read metrics.

S3InputStream, GCSInputStream, and ADLSInputStream already guard against this (fixed in #16055); OSSInputStream and EcsSeekableInputStream were the two implementations left over from that fix. This PR applies the same guard to both.

A previous attempt at this exact fix (#16266) was opened but auto-closed for inactivity rather than rejected on merit — this PR picks that up with tests added for both the single-byte and buffered read() overloads on each class.

Closes #16062

What user-facing changes does this include?

None. This only affects internal position tracking and read metrics inside OSSInputStream and EcsSeekableInputStream when a read reaches EOF; there is no change to any public API, table format, or configuration.

Are there any user-facing changes?

  • Fixes metrics reporting a negative byte count when a buffered read hits EOF.
  • Fixes internal position tracking so a stream that has hit EOF doesn't end up with a position off by one byte relative to the actual object length.

How was this change tested?

Added testReadSingleEOF and testReadBufferedEOF to both TestOSSInputStream and TestEcsSeekableInputStream, mirroring the tests added for S3InputStream/GCSInputStream/ADLSInputStream in #16055.
Verified locally that all four new tests fail against the pre-fix code and pass after the fix; ran spotlessCheck for both modules.

OSSInputStream and EcsSeekableInputStream advanced pos/next and
incremented read metrics before checking whether the underlying stream
had reached EOF (-1). This corrupted the tracked position by one byte
and passed a negative count to the read-bytes counter. GCSInputStream,
S3InputStream, and ADLSInputStream already guard against this; apply
the same fix here.
@damansingh1313

Copy link
Copy Markdown
Contributor Author

Hi @anoopj @amogh-jahagirdar,
Please review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

EOF not handled correctly in single byte read in cloud storage InputStream implementations

1 participant