dbtsai commented on code in PR #17236:
URL: https://github.com/apache/iceberg/pull/17236#discussion_r3593262155
##########
gcp/src/main/java/org/apache/iceberg/gcp/gcs/AnalyticsCoreUtil.java:
##########
@@ -200,6 +211,21 @@ public void readVectored(List<FileRange> ranges,
IntFunction<ByteBuffer> allocat
.setByteBufferFuture(fileRange.byteBuffer())
.build())
.collect(Collectors.toList());
+ // readVectored only schedules the reads; record metrics as each range
future completes
+ // successfully so that failed ranges are not counted. Count the bytes
actually delivered
+ // (the completed buffer is flipped for reading) rather than the
requested length, which can
+ // differ on a short read near EOF.
+ for (FileRange range : ranges) {
+ range
+ .byteBuffer()
+ .thenAccept(
+ buffer -> {
+ if (buffer != null && buffer.remaining() > 0) {
+ readBytes.increment(buffer.remaining());
Review Comment:
Good catch — you're right, and I couldn't refute it. `HadoopMetricsContext`
maps `READ_BYTES` to `FileSystem.Statistics.incrementBytesRead`, and Hadoop's
`Statistics` accumulates in a `ThreadLocal<StatisticsData>`; Spark attributes
task input bytes from the task thread's slot. My `thenAccept` callback runs on
whichever thread completes the future (an analytics-core background thread), so
on this path the bytes would land on the wrong thread's `Statistics` and never
reach Spark's task metrics — a silent no-op for exactly the scenario #17208
targets. And as you note, the `DefaultMetricsContext`/`LongAdder` test can't
surface thread attribution, so it stayed green.
I'll switch this path to count synchronously on the caller thread before
delegating to `stream.readVectored(...)`, matching every other counting site in
the PR. The tradeoff is that, unlike the completion-callback approach,
synchronous counting can only use the requested `range.length()` (the delivered
size isn't known until the async read finishes), so it loses the failed-range /
short-read precision this code was trying to get. Given the goal is that the
bytes reach Spark at all, correct-thread attribution is the more important
property; I'll count `range.length()` synchronously and add a comment
documenting the imprecision. I'll also drop the now-misleading short-read test
assertion for this path.
Does counting requested length synchronously (with a comment on the
imprecision) sound right to you, or would you prefer I document the async
attribution as a known limitation instead and leave counting in the callback?
Happy to go either way.
--
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]
