viirya commented on code in PR #17236:
URL: https://github.com/apache/iceberg/pull/17236#discussion_r3593200215
##########
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:
This counts inside a `thenAccept` callback that runs on whichever thread
completes the future — an analytics-core background thread, not the caller.
Every other counting site in this PR increments synchronously on the caller
(task) thread.
That matters for the stated goal: under `HadoopMetricsContext`, `READ_BYTES`
→ `FileSystem.Statistics.incrementBytesRead`, and Hadoop's `Statistics`
accumulates per-thread, with Spark attributing task input bytes from the *task
thread's* statistics. If these futures complete on a background thread, the
bytes land on the wrong thread's `Statistics` and may never reach Spark's task
metrics — so the fix could silently no-op on this specific analytics-core path,
which is the scenario #17208 is about.
`TestAnalyticsCoreUtil` uses `DefaultMetricsContext` (a plain `LongAdder`),
so it can't surface a thread-attribution issue. Could you confirm the behavior
against a real `HadoopMetricsContext`? If the async attribution is inherent to
analytics-core, counting `range.length()` synchronously before delegating
(accepting the failed-range imprecision this code deliberately avoids), or
documenting it as a known limitation, would be worth weighing.
--
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]
