Peter Rozsa 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 3: (3 comments) > Patch Set 3: > > (2 comments) > > Went through PS3: the FileIO, the translate layer, the metaprovider wiring > and the new tests. Checked the Iceberg side against iceberg-core 1.10.2 > rather than from memory. > > The first thing I'd look at is the DESCRIPTOR_ONLY condition that came out of > getTIcebergTable(), since the FULL form is what COPY TESTCASE serializes into > a file and the vended keys now travel with it - details in the thread on > FeIcebergTable. Beyond that: the write half of the FileIO still resolves > through getConf(), which meets the REST INSERT path landing in 24682; the > prefix match still has no path boundary; and nothing closes the cached > FileSystems now that the removal listener is gone. The rest are nits. Thanks, Aleksandr! I'd like to ask you to make your AI reviews less verbose, and please check every claim that the AI states as it could be misleading. 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: (Credential cred : icebergTable.getCrede > The condition had a reason, even if it wasn't written down: DESCRIPTOR_ONLY Wow, your AI gave the same output as my AI gave me before pushing PS3, but the reality is that COPY TESTCASE was never used for bug reports and also the vended credentials are short-living credentials, so those would be expired when the theoretical bug report is created. 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: * for it, rather than whatever the shared Hadoop FileSystem cache happens to hold. > That holds for everything reaching this FileIO today, but it stops holding Let's handle REST catalog writing support and this patch separately, I would like to focus on the credential vending part in this patch, and if the REST writing support lands before this one, I'll adjust it 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 long FS_CACHE_TTL_MINUTES = 10; > That settles the use-after-close. What's left is that nothing closes them a Yes, it's intended, this is the Hadoop-like behavior, and I think the review history is sufficient for now. -- 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: 3 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 21:19:39 +0000 Gerrit-HasComments: Yes
