Aleksandr Efimov has posted comments on this change. ( http://gerrit.cloudera.org:8080/24755 )
Change subject: IMPALA-15144: Fetch and store credentials from REST catalog ...................................................................... Patch Set 2: (4 comments) Read through the credential path - extraction, translation, thrift - and the FileIO. Four comments; the two on VendedCredentialsFileIO are the ones I'd care about. On coverage: IcebergRestCatalogTest already overrides RESTCatalogAdapter.execute(), and LoadTableResponse.Builder has addCredential() in 1.10.2, so having the test server vend a fake credential looks reachable, and it would cover extract() -> getCredentials() -> thrift end to end. Is that planned for a follow-up? Also curious which side ends up reading expiry_ms - is that the patch that consumes the credentials in the backend? http://gerrit.cloudera.org:8080/#/c/24755/2/fe/src/main/java/org/apache/impala/catalog/iceberg/VendedCredentialsFileIO.java File fe/src/main/java/org/apache/impala/catalog/iceberg/VendedCredentialsFileIO.java: http://gerrit.cloudera.org:8080/#/c/24755/2/fe/src/main/java/org/apache/impala/catalog/iceberg/VendedCredentialsFileIO.java@49 PS2, Line 49: public class VendedCredentialsFileIO extends HadoopFileIO Only the two newInputFile() overloads are overridden here. In Iceberg 1.10.2 the rest of HadoopFileIO's DelegateFileIO surface still resolves through getConf(): newOutputFile() ends in HadoopOutputFile.fromPath(path, getConf()), deleteFile(), listPrefix() and deletePrefix() go through Util.getFs(path, getConf()), and deleteFiles() fans out to the same deleteFile(). All of them use the shared FileSystem cache without the vended credential. That surface is used in-tree - ImpalaIcebergDeleteOrphanFiles has table.io().deleteFile() - though I'm not sure a REST catalog table can reach it today. Either way the fallback is silent: on a bucket that needs the vended credential it fails, and on one the ambient credentials also cover it quietly uses those instead. Would it make sense to route them through fileSystemFor() too, or to throw there so the gap stays visible? http://gerrit.cloudera.org:8080/#/c/24755/2/fe/src/main/java/org/apache/impala/catalog/iceberg/VendedCredentialsFileIO.java@57 PS2, Line 57: private static final Cache<String, FileSystem> FS_CACHE = closeEvicted() closes the FileSystem on eviction, but HadoopInputFile keeps the instance it was built with (private final FileSystem fs in 1.10.2) and newStream() calls fs.open() on that field, so the filesystem is used when the file is read, not when it is created. An eviction - by maximumSize=100, or after ten idle minutes on that key - can then close a FileSystem an outstanding InputFile still holds, and the read fails with "Filesystem closed" rather than anything credential-shaped. Dropping the close avoids that at the cost of releasing the connection pool later; a refcount would be exact but more code. Do you have a preference? http://gerrit.cloudera.org:8080/#/c/24755/2/fe/src/main/java/org/apache/impala/catalog/iceberg/VendedCredentialsFileIO.java@135 PS2, Line 135: private Credential findCredential(String location) { The match is a raw startsWith, so a credential vended for s3://bucket/tab also covers s3://bucket/table2/... . Longest-prefix makes that unlikely in practice - should the match require a path boundary anyway? http://gerrit.cloudera.org:8080/#/c/24755/2/fe/src/main/java/org/apache/impala/catalog/iceberg/translate/CredentialScheme.java File fe/src/main/java/org/apache/impala/catalog/iceberg/translate/CredentialScheme.java: http://gerrit.cloudera.org:8080/#/c/24755/2/fe/src/main/java/org/apache/impala/catalog/iceberg/translate/CredentialScheme.java@37 PS2, Line 37: public class CredentialScheme { Nothing exercises this class in the patch: ConfigTranslatorTest builds its own MapRenameRule/ProgrammaticRule instances with s3-like keys, so the registry itself - which Iceberg key becomes which fs.* key, the scheme parsing in forPrefix(), the expiry units - is uncovered. A small test per scheme, plus an unknown scheme returning null, would pin down what the backend ends up consuming. -- To view, visit http://gerrit.cloudera.org:8080/24755 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1d9c6e97e17fe8ad6304b49b07fd047cc2a3ffbe Gerrit-Change-Number: 24755 Gerrit-PatchSet: 2 Gerrit-Owner: Peter Rozsa <[email protected]> Gerrit-Reviewer: Aleksandr Efimov <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Nandor Kollar <[email protected]> Gerrit-Reviewer: Peter Rozsa <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 26 Aug 2026 16:07:44 +0000 Gerrit-HasComments: Yes
