GCS, S3, ADLS: Handle EOF in inputStreams - #16055
Conversation
|
I filed #16062 to keep track of this general problem. |
What do you think will be better PR per cloud storage or one PR for all of them? |
Well, it's not a lot of code... I will add S3 and ADLS here. |
vladislav-sidorovich
changed the title
|
I've pinged @nastra @danielcweeks @amogh-jahagirdar |
amogh-jahagirdar
left a comment
amogh-jahagirdar
left a comment
There was a problem hiding this comment.
Thanks @vladislav-sidorovich and @anoopj for reviewing! This looks right to me, but I think we should also get @danielcweeks input on this as well. I'm curious if you've seen issues with EOF exceptions in practice? My guess is so far we've largely been getting lucky because we issue targeted range reads / always reading the appropriate length from metadata.
I was only reviewing GCP module to check if we have any issues and find it. I believe in PROD usage buffered reader was used, so the problems were minor metrics inaccuracies. |
steveloughran
left a comment
steveloughran
left a comment
There was a problem hiding this comment.
these stores can return EOF if
- you've run out of data
- the connection is broken
- bad stuff happening on the http channel, especially if wildfly and openssl is involved
s3afs input stream really tries to differentiate End of File and End of Network connection, as that can be retried.
@danielcweeks would you be able to take a look on the PR? |
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
|
should be fixed, rather than closed. important in production, as at scale these fails will happen daily |
|
Not stale |
@danielcweeks would you be able to take a look on the PR? |
danielcweeks
left a comment
danielcweeks
left a comment
There was a problem hiding this comment.
Thanks everyone for the reviews, this looks correct to me.
…tStreamWrapper In read(byte[], int, int), readOperations was incremented unconditionally, including on EOF (bytesRead == -1), contradicting the read() method above it and the EOF-handling convention from apache#16055. Guard both counters on != -1 so an EOF read counts neither bytes nor an operation. Co-authored-by: Isaac
Review feedback on the read-metrics instrumentation: - AnalyticsCoreUtil.readVectored: count range.length() synchronously on the caller thread instead of in the range-future completion callback. The futures complete on analytics-core background threads; under HadoopMetricsContext, READ_BYTES accumulates per-thread and Spark reads task input from the task thread, so callback counting would land the bytes on the wrong thread and never reach Spark's task metrics (viirya). - readTail on S3/GCS/ADLS: guard readOperations together with readBytes on bytesRead > 0 so an empty-tail read counts neither (szehon-ho). - AnalyticsCoreUtil.read()/read(byte[],int,int): guard both counters on bytesRead != -1 for consistency with the rest of the PR and apache#16055 (JoshRosen). - TestS3InputStream: drop the shared lenient() getObject stub and stub inline in the two tests that need it, restoring strict stubbing (szehon-ho). A codebase-wide scan for the same pattern found two more streams with the same bug (both pre-existing): Aliyun OSSInputStream and Dell EcsSeekableInputStream incremented read metrics unconditionally, so an EOF read over-counted an operation and, in the buffered path, readBytes.increment(-1) decremented the byte counter and corrupted pos. Both now read first and return early on EOF before touching pos/counters. Co-authored-by: Isaac
5 participants
Returned
EOFvalue was ignored and stale byte fromsingleByteBufferwas retuned inread().In
read(byte[] b, int off, int len)behavior was correct, only metrics andposwere affected.