steveloughran commented on code in PR #15029:
URL: https://github.com/apache/iceberg/pull/15029#discussion_r2832849093
##########
gcp/src/main/java/org/apache/iceberg/gcp/gcs/GCSInputStream.java:
##########
@@ -86,6 +88,24 @@ private void openStream() {
channel = openChannel();
}
+ private void handleStorageException(StorageException storageException)
throws NotFoundException {
+ if (storageException.getCode() == 404) {
+ throw new NotFoundException(storageException, "Blob does not exist: %s",
blobId);
+ }
+ LOG.error("Storage operation failed for blob {}", blobId,
storageException);
Review Comment:
does that need logging here?
##########
gcp/src/main/java/org/apache/iceberg/gcp/gcs/GCSInputStream.java:
##########
@@ -127,7 +150,11 @@ public int read() throws IOException {
singleByteBuffer.position(0);
pos += 1;
- channel.read(singleByteBuffer);
+ try {
+ channel.read(singleByteBuffer);
+ } catch (IOException ioException) {
+ handleIOException(ioException);
Review Comment:
nit, note that the line below is never reached, so someone/something reading
the code knows the flow without looking into handleIOException
##########
gcp/src/main/java/org/apache/iceberg/gcp/gcs/GCSInputStream.java:
##########
@@ -161,20 +193,35 @@ public void readFully(long position, byte[] buffer, int
offset, int length) thro
@Override
public int readTail(byte[] buffer, int offset, int length) throws
IOException {
if (blobSize == null) {
- blobSize = storage.get(blobId).getSize();
+ try {
+ blobSize = storage.get(blobId).getSize();
+ } catch (StorageException storageException) {
+ handleStorageException(storageException);
+ }
}
long startPosition = Math.max(0, blobSize - length);
try (ReadChannel readChannel = openChannel()) {
readChannel.seek(startPosition);
return read(readChannel, ByteBuffer.wrap(buffer), offset, length);
+ } catch (StorageException e) {
+ if (e.getCode() == 404) {
+ throw new NotFoundException(e, "Blob does not exist: %s", blobId);
+ }
+ LOG.error("Failed to read from blob {}", blobId, e);
+ throw new IOException(e);
Review Comment:
and here a StorageException is mapped to NotFoundException or an IOE. This
is inconsistent
##########
gcp/src/main/java/org/apache/iceberg/gcp/gcs/GCSInputStream.java:
##########
@@ -161,20 +193,35 @@ public void readFully(long position, byte[] buffer, int
offset, int length) thro
@Override
public int readTail(byte[] buffer, int offset, int length) throws
IOException {
if (blobSize == null) {
- blobSize = storage.get(blobId).getSize();
+ try {
+ blobSize = storage.get(blobId).getSize();
+ } catch (StorageException storageException) {
+ handleStorageException(storageException);
Review Comment:
so here a StorageException is mapped to NotFoundException or RuntimeException
--
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]
