williamhyun commented on PR #17457: URL: https://github.com/apache/iceberg/pull/17457#issuecomment-5203326426
Thank you @steveloughran for the thoughtful comments, 1. I'm on board with the direction for the end state error handling model having classifying vs handling separation. Given that, I'd like to keep this PR to the current minimal handling and do the RTE mapping as a dedicated follow-up: a generic HTTP-status -> RTE mapper in `core` to keep it cloud-agnostic, and the S3-specific bits (throttle-403 detection, isThrottlingException, the OpenSSL exception surface) layered in `aws` where the SDK types are available. Happy to open a tracking issue for that and tag you, would love your input on the design. 2. The 4MB block size came from matching [ContentCache.BUFFER_CHUNK_SIZE](https://github.com/apache/iceberg/blob/c9afdc64f41e42d4601fb3aa02ebe8cd785f453a/core/src/main/java/org/apache/iceberg/io/ContentCache.java#L53), which is the precedent for a read chunk here, so I went with that for consistency. I see that 8MB is a [common ecosystem default](https://hadoop.apache.org/docs/r3.4.1/hadoop-aws/tools/hadoop-aws/prefetching.html) so I'm not strongly opinionated here. Happy to bump it to 8MB if you'd prefer we align with that convention. 3. Agreed that parquet wants parallel ranged reads. Currently, `HTTPInputStream` inherits the default `readVectored`, which runs them serially, which avoids prefetch but doesn't yet use the separate threads for parallel reading. Puffin is more nuanced: * Deletion Vectors: prefetch is bypassed entirely. The DV DeleteFile has exact `contentOffset/contentSizeInBytes`, so `readDV` does one positional `RangeReadable.readFully` for exactly the blob. Parallelism comes from the delete worker pool (one GET per DV, file/task level, not an intra-file batch). * Stats (NDV): no file read. * Bulk blob scans (readAll): the one path that prefetches and benefits from it. Blobs are written contiguously and read in order, so read-ahead coalesces them into fewer GETs. Only caller is the offline rewrite-table-path. * For this PR, I'd propose we keep the read-ahead prefetch for the sequential consumers (Avro + Puffin bulk scans) and leave `readVectored` on its serial default for now. Noting that `S3InputStream` also relies on the serial default today, this is consistent rather than a regression. Parallel `readVectored` will be a great followup for parquet reading performance. -- 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]
