Re: [PR] AWS, GCP, Azure: Track read metrics for positioned, vectored, and accelerated reads [iceberg]

["dbtsai (via GitHub)" <gi...@apache.org>]

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: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org