dbtsai commented on code in PR #17236:
URL: https://github.com/apache/iceberg/pull/17236#discussion_r3607109934
##########
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:
Done — switched this path to count synchronously on the caller thread before
delegating to `stream.readVectored(...)`, using the requested `range.length()`,
matching every other counting site in the PR. Added a comment documenting the
bounded over-count on short reads / failed ranges, and flipped the test to
`readVectoredCountsRequestedLengthSynchronously` (asserts both counters move up
front, before the futures complete). Also noted in the test that
`DefaultMetricsContext` (a plain `LongAdder`) cannot verify per-thread
`Statistics` attribution, so the correctness argument rests on the
`HadoopMetricsContext` reasoning rather than the unit test. Thanks for catching
this.
--
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]
