Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/22353 )
Change subject: IMPALA-13586: Initial support for Iceberg REST Catalogs ...................................................................... Patch Set 13: (13 comments) Thanks for the comments! http://gerrit.cloudera.org:8080/#/c/22353/2/be/src/service/frontend.h File be/src/service/frontend.h: http://gerrit.cloudera.org:8080/#/c/22353/2/be/src/service/frontend.h@93 PS2, Line 93: TGetCatalogInfoResult* > Pass by reference We usually pass output parameters by pointers, see above. http://gerrit.cloudera.org:8080/#/c/22353/2/be/src/service/frontend.cc File be/src/service/frontend.cc: http://gerrit.cloudera.org:8080/#/c/22353/2/be/src/service/frontend.cc@261 PS2, Line 261: > Pass by reference We pass output parameters by pointers. http://gerrit.cloudera.org:8080/#/c/22353/13/be/src/service/impala-http-handler.cc File be/src/service/impala-http-handler.cc: http://gerrit.cloudera.org:8080/#/c/22353/13/be/src/service/impala-http-handler.cc@1001 PS13, Line 1001: Value error(status.GetDetail().c_str(), document->GetAllocator()); > nit: this error reporting code piece is used multiple times through this fi There are indeed lots of identical code pieces, but there are also code pieces that are very similar to this one, but have minor differences. This CR is already quite large, and I rather not introduce any bugs with it. http://gerrit.cloudera.org:8080/#/c/22353/2/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/22353/2/be/src/service/impala-server.cc@3136 PS2, Line 3136: calloncehack::InitializeCallOnceHack(); > Add a TODO and JIRA# to support workload management. One way to support this is via multi-catalog setups where we have CatalogD. But I also opened IMPALA-13830 to support workload management in lightweight deployments. http://gerrit.cloudera.org:8080/#/c/22353/2/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergRESTCatalog.java File fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergRESTCatalog.java: http://gerrit.cloudera.org:8080/#/c/22353/2/fe/src/main/java/org/apache/impala/catalog/iceberg/IcebergRESTCatalog.java@54 PS2, Line 54: private static final String KEY_CLIENT_SECRET = "iceberg.rest-catalog.client-secret"; > Does this need to be synchronized? Done http://gerrit.cloudera.org:8080/#/c/22353/13/fe/src/main/java/org/apache/impala/catalog/local/IcebergMetaProvider.java File fe/src/main/java/org/apache/impala/catalog/local/IcebergMetaProvider.java: http://gerrit.cloudera.org:8080/#/c/22353/13/fe/src/main/java/org/apache/impala/catalog/local/IcebergMetaProvider.java@98 PS13, Line 98: DirectMetaProvider > nit: IcebergMetaProvider Done http://gerrit.cloudera.org:8080/#/c/22353/13/fe/src/main/java/org/apache/impala/catalog/local/IcebergMetaProvider.java@128 PS13, Line 128: DirectMetaProvider > nit: IcebergMetaProvider Done http://gerrit.cloudera.org:8080/#/c/22353/13/fe/src/main/java/org/apache/impala/catalog/local/IcebergMetaProvider.java@216 PS13, Line 216: {}.{} > it should be %s.%s Good catch, done. http://gerrit.cloudera.org:8080/#/c/22353/13/fe/src/main/java/org/apache/impala/service/FeCatalogManager.java File fe/src/main/java/org/apache/impala/service/FeCatalogManager.java: http://gerrit.cloudera.org:8080/#/c/22353/13/fe/src/main/java/org/apache/impala/service/FeCatalogManager.java@190 PS13, Line 190: throw new IllegalStateException( > IllegalStateException("Create IcebergMetaProvider failed", e) Done http://gerrit.cloudera.org:8080/#/c/22353/13/fe/src/main/java/org/apache/impala/service/FeCatalogManager.java@200 PS13, Line 200: Preconditions.checkState(catalogConfigDir != null && > It could be checked whether the provided path exists or not Added the precondition check to listFiles(). http://gerrit.cloudera.org:8080/#/c/22353/13/fe/src/main/java/org/apache/impala/service/FeCatalogManager.java@231 PS13, Line 231: "Expected property %s was not specified.", key)); > Add {configFile} to error messages Done http://gerrit.cloudera.org:8080/#/c/22353/13/java/iceberg-rest-catalog-test/src/main/java/org/apache/iceberg/rest/IcebergRestCatalogTest.java File java/iceberg-rest-catalog-test/src/main/java/org/apache/iceberg/rest/IcebergRestCatalogTest.java: http://gerrit.cloudera.org:8080/#/c/22353/13/java/iceberg-rest-catalog-test/src/main/java/org/apache/iceberg/rest/IcebergRestCatalogTest.java@29 PS13, Line 29: import java.net.URLClassLoader; > nit: unused imports Done http://gerrit.cloudera.org:8080/#/c/22353/8/tests/custom_cluster/test_iceberg_rest_catalog.py File tests/custom_cluster/test_iceberg_rest_catalog.py: http://gerrit.cloudera.org:8080/#/c/22353/8/tests/custom_cluster/test_iceberg_rest_catalog.py@29 PS8, Line 29: > flake8: E225 missing whitespace around operator Done -- To view, visit http://gerrit.cloudera.org:8080/22353 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1722b898b568d2f5689002f2b9bef59320cb088c Gerrit-Change-Number: 22353 Gerrit-PatchSet: 13 Gerrit-Owner: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Kurt Deschler <kdesc...@cloudera.com> Gerrit-Reviewer: Noemi Pap-Takacs <npaptak...@cloudera.com> Gerrit-Reviewer: Peter Rozsa <pro...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Tue, 18 Mar 2025 14:09:04 +0000 Gerrit-HasComments: Yes
