viirya commented on code in PR #17236:
URL: https://github.com/apache/iceberg/pull/17236#discussion_r3599524978
##########
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:
Synchronous counting of `range.length()` is the right call. The whole point
of this metric is to reach Spark's per-thread task input statistics via
`HadoopMetricsContext`; if it accumulates on the wrong thread it never arrives,
so the number is effectively zero on this path. Over-counting a short read or a
failed batch by a bounded amount is a precision issue — the magnitude is right
and it lands where Spark can see it. Correct-thread attribution wins over
precision here, so I wouldn't keep the callback and just document it — that
would knowingly ship a broken metric on this path.
Two things to fold in when you switch it:
- Since you'll increment `range.length()` up front, a batch where
`stream.readVectored(...)` throws will have already counted those bytes. That's
acceptable for a metric (bounded over-count on a failure path), but please say
so in the comment so the imprecision is on the record — both the short-read and
the failed-range cases.
- Agreed on dropping the short-read assertion. Worth being explicit in the
test/PR that the remaining `DefaultMetricsContext` test can't verify thread
attribution at all (it's a plain `LongAdder`); the correctness argument here is
the per-thread `Statistics` reasoning, not the unit test. No need to fake a
thread-attribution test — just don't let the green test imply coverage it
doesn't have.
Thanks for the quick turnaround on this and the EOF fix.
--
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]
