szehon-ho commented on code in PR #17236:
URL: https://github.com/apache/iceberg/pull/17236#discussion_r3599633326
##########
gcp/src/main/java/org/apache/iceberg/gcp/gcs/GCSInputStream.java:
##########
@@ -179,7 +181,12 @@ public int readTail(byte[] buffer, int offset, int length)
throws IOException {
long startPosition = Math.max(0, blobSize - length);
try (ReadChannel readChannel = openChannel()) {
readChannel.seek(startPosition);
- return read(readChannel, ByteBuffer.wrap(buffer), offset, length);
+ int bytesRead = read(readChannel, ByteBuffer.wrap(buffer), offset,
length);
+ if (bytesRead > 0) {
+ readBytes.increment(bytesRead);
+ }
+ readOperations.increment();
Review Comment:
`readOperations` is incremented even when `bytesRead == -1` (empty/EOF
tail), while `readBytes` is guarded. That's the same EOF-counting addressed in
#16055, and it's inconsistent with `read()`/`read(byte[])` above. Suggest
guarding both together:
```java
if (bytesRead > 0) {
readBytes.increment(bytesRead);
readOperations.increment();
}
```
and flipping `testReadTailEmptyObjectDoesNotDecrementMetrics` to assert
`readOperations == 0`.
##########
aws/src/main/java/org/apache/iceberg/aws/s3/S3InputStream.java:
##########
@@ -184,8 +186,11 @@ public int readTail(byte[] buffer, int offset, int length)
throws IOException {
String range = String.format("bytes=-%s", length);
- try (InputStream stream = readRange(range)) {
- return IOUtil.readRemaining(stream, buffer, offset, length);
+ try (InputStream rangeStream = readRange(range)) {
+ int bytesRead = IOUtil.readRemaining(rangeStream, buffer, offset,
length);
+ readBytes.increment(bytesRead);
+ readOperations.increment();
Review Comment:
Same convention nit: `IOUtil.readRemaining` returns `0` at EOF, so an empty
tail read still ticks `readOperations` (and `readBytes.increment(0)`). Guard
both on `bytesRead > 0` to stay consistent with `read()` and #16055.
##########
azure/src/main/java/org/apache/iceberg/azure/adlsv2/ADLSInputStream.java:
##########
@@ -192,7 +194,10 @@ public int readTail(byte[] buffer, int offset, int length)
throws IOException {
long readStart = fileSize - length;
try (InputStream inputStream = openRange(new
FileRange(readStart)).getInputStream()) {
- return IOUtil.readRemaining(inputStream, buffer, offset, length);
+ int bytesRead = IOUtil.readRemaining(inputStream, buffer, offset,
length);
+ readBytes.increment(bytesRead);
+ readOperations.increment();
Review Comment:
Same as S3/GCS — an empty tail read counts an operation. Guard
`readBytes`/`readOperations` on `bytesRead > 0` so a no-data read doesn't
increment (#16055 convention).
##########
aws/src/test/java/org/apache/iceberg/aws/s3/TestS3InputStream.java:
##########
@@ -44,7 +53,9 @@ public final class TestS3InputStream {
@BeforeEach
void before() {
- when(s3Client.getObject(any(GetObjectRequest.class),
any(ResponseTransformer.class)))
+ // lenient: the metrics tests re-stub getObject with their own data streams
+ lenient()
Review Comment:
The two new metrics tests re-stub `getObject` with their own data, so this
shared stub goes unused there and would trip `UnnecessaryStubbingException` —
hence `lenient()`. Reasonable, but since only `testReadTailClosesTheStream`
needs the default stub, a cleaner option is to drop it from `@BeforeEach` and
stub `getObject` inline in that one test, keeping strict stubbing everywhere.
Non-blocking.
##########
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:
+1 on synchronous `range.length()` counting — agreed it's the right
correct-thread-vs-precision tradeoff.
Related convention point: the same "don't count on a no-data read" rule from
#16055 isn't applied in `readTail` on the three streams — `readBytes` is
guarded but `readOperations` still increments when the tail read returns
`-1`/`0` (empty object). I left inline comments; worth folding a `bytesRead >
0` guard into this PR while we're here.
--
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]
