Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/23156 )
Change subject: IMPALA-14018: Configure OAUTH2 with Lakekeeper and fix Impala's config handling ...................................................................... Patch Set 3: (5 comments) Thanks for the comments! http://gerrit.cloudera.org:8080/#/c/23156/2/fe/src/main/java/org/apache/impala/catalog/iceberg/RESTCatalogProperties.java File fe/src/main/java/org/apache/impala/catalog/iceberg/RESTCatalogProperties.java: http://gerrit.cloudera.org:8080/#/c/23156/2/fe/src/main/java/org/apache/impala/catalog/iceberg/RESTCatalogProperties.java@254 PS2, Line 254: Preconditions.checkState(!finalMap_.containsKey(entry.getKey())); > We could assert that the return value of finalMap_.put() is NULL instead. There is no Preconditions.checkNull(), so I could only write: Preconditions.checkState( finalMap_.put(entry.getKey(), entry.getValue()) == null); Or: String prevItem = finalMap_.put(entry.getKey(), entry.getValue()); Preconditon.checkState(prevItem == null); Which is not so readable I think, and in code like this, that executes only once during startup, I'd prefer readability. http://gerrit.cloudera.org:8080/#/c/23156/2/fe/src/test/java/org/apache/impala/catalog/iceberg/TestRESTCatalogProperties.java File fe/src/test/java/org/apache/impala/catalog/iceberg/TestRESTCatalogProperties.java: http://gerrit.cloudera.org:8080/#/c/23156/2/fe/src/test/java/org/apache/impala/catalog/iceberg/TestRESTCatalogProperties.java@40 PS2, Line 40: // RESTCatalogPro > This assertion is redundant. Also applies to other tests. Done http://gerrit.cloudera.org:8080/#/c/23156/2/fe/src/test/java/org/apache/impala/catalog/iceberg/TestRESTCatalogProperties.java@116 PS2, Line 116: when the same property is defined > Here the problem is that properties are defined with two alternative names. Done http://gerrit.cloudera.org:8080/#/c/23156/2/fe/src/test/java/org/apache/impala/catalog/iceberg/TestRESTCatalogProperties.java@148 PS2, Line 148: when a verified config doesn't > Here the problem is that a config has an incorrect value. Done http://gerrit.cloudera.org:8080/#/c/23156/2/fe/src/test/java/org/apache/impala/catalog/iceberg/TestRESTCatalogProperties.java@166 PS2, Line 166: iceberg.catalog.type > This is the ignored value, right? We could check that it is not present in It was already checked implicitly, as we checked the size of restProps.getCatalogProperties(), then we checked each key and value. But now I also added explicit check about these for readability. -- To view, visit http://gerrit.cloudera.org:8080/23156 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie5785cb72773e188b1de7c7924cc6f0b1f96de33 Gerrit-Change-Number: 23156 Gerrit-PatchSet: 3 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Peter Rozsa <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 16 Jul 2025 16:00:45 +0000 Gerrit-HasComments: Yes
