dbtsai commented on code in PR #17236:
URL: https://github.com/apache/iceberg/pull/17236#discussion_r3607115501
##########
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:
Done — guarded both counters on `bytesRead > 0` and flipped the empty-object
test 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:
Done, guarded both on `bytesRead > 0`.
##########
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:
Done, same guard.
##########
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:
Dropped the shared stub from `@BeforeEach` and stubbed inline in the two
tests that need it, so strict stubbing everywhere. Thanks.
##########
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 point — folded the `bytesRead > 0` guard into `readTail` on all three
streams in this PR.
--
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]
