Zoltan Borok-Nagy 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: (10 comments) Left a few comments, but looks great overall! http://gerrit.cloudera.org:8080/#/c/24755/2/common/thrift/CatalogObjects.thrift File common/thrift/CatalogObjects.thrift: http://gerrit.cloudera.org:8080/#/c/24755/2/common/thrift/CatalogObjects.thrift@701 PS2, Line 701: struct TCredential { Please add comments for this structure and its members. http://gerrit.cloudera.org:8080/#/c/24755/2/common/thrift/CatalogObjects.thrift@704 PS2, Line 704: expiry_ms nit: 'expires_at_ms' is more clear and it's closer to what being used by different object stores 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: type == ThriftObjectType.DESCRIPTOR_ONLY Why is it only set for DESCRIPTOR_ONLY? 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@65 PS2, Line 65: IcebergRESTCatalog.extractCredentials Credential.extract() in IcebergMetaProvider? http://gerrit.cloudera.org:8080/#/c/24755/2/fe/src/main/java/org/apache/impala/catalog/iceberg/VendedCredentialsFileIO.java@120 PS2, Line 120: ( nit: missing space http://gerrit.cloudera.org:8080/#/c/24755/2/fe/src/main/java/org/apache/impala/catalog/iceberg/VendedCredentialsFileIO.java@143 PS2, Line 143: Credential cred = new Credential(storageCred.prefix(), storageCred.config()); We could create a member list of Credential objects from storageCredentials_, and use it in findCredential() to avoid reconstructing the same credentials (and hadoop config) multiple times. 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@39 PS2, Line 39: private static final String S3_ACCESS_KEY_ID = "s3.access-key-id"; Can you add a link to the source of these configs, e.g. is it https://github.com/apache/iceberg/blob/main/open-api/rest-catalog-open-api.yaml#L3873 ? http://gerrit.cloudera.org:8080/#/c/24755/2/fe/src/main/java/org/apache/impala/catalog/iceberg/translate/CredentialScheme.java@65 PS2, Line 65: private static final String GCS_OAUTH2_TOKEN = "gcs.oauth2.token"; Please add a link to the source of these configs. http://gerrit.cloudera.org:8080/#/c/24755/2/fe/src/main/java/org/apache/impala/catalog/iceberg/translate/TranslationRule.java File fe/src/main/java/org/apache/impala/catalog/iceberg/translate/TranslationRule.java: http://gerrit.cloudera.org:8080/#/c/24755/2/fe/src/main/java/org/apache/impala/catalog/iceberg/translate/TranslationRule.java@33 PS2, Line 33: public interface TranslationRule { Nice refactoring! http://gerrit.cloudera.org:8080/#/c/24755/2/fe/src/test/java/org/apache/impala/catalog/iceberg/CredentialTest.java File fe/src/test/java/org/apache/impala/catalog/iceberg/CredentialTest.java: http://gerrit.cloudera.org:8080/#/c/24755/2/fe/src/test/java/org/apache/impala/catalog/iceberg/CredentialTest.java@93 PS2, Line 93: } Unit tests could be added for * toHadoopConfig() * getExpiryMs() * identity() -- 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 11:19:13 +0000 Gerrit-HasComments: Yes
