vbhanuchander-lang commented on PR #16894:
URL: https://github.com/apache/iceberg/pull/16894#issuecomment-5528034230
@thswlsqls the stale bot closed this on 31 July without anyone reviewing it.
The bug is still on
`main` and the fix is still right, but **it no longer applies** — posting
the verification and the
rebase so it is cheap to pick back up.
**The bug and your parity argument both hold on `main`.**
`GCSInputStream.readTail` still calls the
private `read()` helper once, which does a single
`readChannel.read(buffer)`; a NIO `ReadChannel` is
allowed to return fewer bytes than requested. Meanwhile:
- `S3InputStream.readTail` → `IOUtil.readRemaining(rangeStream, buffer,
offset, length)`
- `ADLSInputStream.readTail` → `IOUtil.readRemaining(inputStream, buffer,
offset, length)`
both of which loop. So GCS is the only one of the three that can return
short, and
`RangeReadable.readTail` is documented as *"Read the last `length` bytes
from the file"*.
**Why it stopped applying:** `readTail` gained read metrics after you wrote
this —
```java
int bytesRead = read(readChannel, ByteBuffer.wrap(buffer), offset, length);
if (bytesRead > 0) {
readBytes.increment(bytesRead);
readOperations.increment();
}
return bytesRead;
```
so the patch context is gone, and `TestGCSInputStream` also gained
`testRangeReadMetrics` and
`testReadTailEmptyObjectDoesNotDecrementMetrics`, which collide with where
your test lands. The
resolution is small — keep your loop and count once from the total:
```java
ByteBuffer wrapped = ByteBuffer.wrap(buffer);
int totalRead = 0;
while (totalRead < length) {
int read = read(readChannel, wrapped, offset + totalRead, length -
totalRead);
if (read < 0) {
break;
}
totalRead += read;
}
if (totalRead > 0) {
readBytes.increment(totalRead);
readOperations.increment();
}
return totalRead;
```
**Verified against current `main`:** with that rebase `:iceberg-gcp:test
--tests '*TestGCSInputStream'`
is **9 tests, 0 failures** — including the two newer metrics tests, and the
empty-object one still
gets `0` and increments nothing. Reverting only `GCSInputStream.java` and
keeping your test fails
with `expected: 16 but was: 8`, so the test does guard the change rather
than passing either way.
**One suggestion, and it is theoretical rather than a defect.** The loop
breaks on `read < 0`, which
matches `IOUtil.readRemaining` — but that helper wraps `InputStream.read`,
which can only return 0
when the requested length is 0, whereas a NIO channel is permitted to return
0 when no bytes are
immediately available. GCS's `ReadChannel` is blocking so I do not think it
can happen in practice,
but breaking on `read <= 0` (or guarding on no progress) costs nothing and
removes the possibility
of a spin. Your call — I would not hold the fix for it.
@kevinjqliu @danielcweeks you have both merged in `gcp/` recently — would
one of you be willing to
reopen this? It is +49/-1 in one module with a test, and the change is
@thswlsqls's.
--
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]
