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: (5 comments) http://gerrit.cloudera.org:8080/#/c/24755/2/fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java File fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java: http://gerrit.cloudera.org:8080/#/c/24755/2/fe/src/main/java/org/apache/impala/catalog/FeIcebergTable.java@910 PS2, Line 910: if (type == ThriftObjectType.DESCRIPTOR_ONLY) { > It's AI slop, I removed it. The condition had a reason, even if it wasn't written down: DESCRIPTOR_ONLY is what reaches the executors, while the FULL form of a LocalIcebergTable has exactly one consumer - FeCatalogUtils.feTableToThrift() (FeCatalogUtils.java:431, its Iceberg branch at 463), reached only from CopyTestCaseStmt.getTestCaseData(). With the condition gone, COPY TESTCASE TO '<dir>' over a REST-backed table writes fs.s3a.access.key, fs.s3a.secret.key and the session token into the testcase file - the file people attach to bug reports. It needs VIEW_METADATA on the table plus ALL on the target URI (CopyTestCaseStmt.java:106 and 122-129), so it also turns metadata access into storage access. Bringing the condition back with a comment naming the reason would cover it; so would filtering the credentials out in feTableToThrift(), if you'd rather keep getTIcebergTable() uniform. 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 > It's only used during scanning, no table operation will ever use this IO, i That holds for everything reaching this FileIO today, but it stops holding inside the window this patch lands in. 24682 adds a coordinator-side INSERT finalizer: Frontend.finalizeIcebergDml() takes the table from the same IcebergMetaProvider, IcebergUtil.getIcebergTransaction() calls newTransaction() on it, and the Append goes through SnapshotProducer, which writes the manifest (newManifestOutputFile) and the manifest list (manifestListPath) via ops.io().newOutputFile(). In 1.10.2 that ends in HadoopFileIO.newOutputFile -> HadoopOutputFile.fromPath(path, getConf()) - the shared FileSystem cache, not the vended credential. deleteFile/listPrefix/deletePrefix/deleteFiles resolve through getConf() as well; those are out of reach for a REST table today, since IcebergRESTCatalog only exists under IcebergMetaProvider and catalogd has no REST catalog type, but the INSERT path isn't. fileSystemFor() already returns the right FileSystem and HadoopOutputFile.fromPath(Path, FileSystem) exists, so newOutputFile is a couple of lines; the delete/list side is more work. The other route is to let the mutating methods throw here and pick them up in a follow-up, so a write can't quietly go out under ambient credentials. Both work for me - the case worth not leaving open is a manifest written by the wrong identity, since that surfaces from inside a commit. Worth noting that vending and REST INSERT haven't met in a test yet either. 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 = > I removed the removalListener That settles the use-after-close. What's left is that nothing closes them at all now: newFileSystem() uses FileSystem.newInstance(), so every entry owns its instance, and an entry dropped by maximumSize=100 or by the ten-minute idle expiry is orphaned - for S3A that's an S3 client and its thread pools alive until GC. Is that the intended trade? Closing on eviction after a grace period, or refcounting the outstanding InputFiles, would avoid it; a line in the class comment saying the instances are deliberately never closed would be enough for me too. 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) { > Done I think this one slipped through: PS3 moved the longest-prefix search into a presorted list, but the filter is still location.startsWith(cred.getPrefix()) (line 159 in PS3), so a credential vended for s3://bucket/tab keeps covering s3://bucket/table2/... . Requiring the next character to be '/' - or the prefix to end with one - would settle it. 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 { > It's used in the Credential ctor, I added translation-related tests The new CredentialTest cases cover what I was after on the translation side: both spellings, the provider only when a session token is present, dropped keys, expiry parsing, identity. Two small ones are still open if you want them - s3n is registered but never exercised, and schemeOf() lowercases the scheme while no test passes an uppercase prefix. The wider gap is one level up: VendedCredentialsFileIO has no test at all, so the part that picks the credential - longest prefix wins, a credential translating to nothing is skipped, no match falls back to super - isn't pinned anywhere. findCredential() is private; making it package-private would make that testable without a real FileSystem. -- 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: Thu, 27 Aug 2026 20:37:03 +0000 Gerrit-HasComments: Yes
