Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/22878 )
Change subject: IMPALA-14016: Add multi-catalog support for local catalog mode ...................................................................... Patch Set 10: (4 comments) Found a few nits, otherwise LGTM! http://gerrit.cloudera.org:8080/#/c/22878/10/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/22878/10/java/iceberg-rest-catalog-test/src/main/java/org/apache/iceberg/rest/IcebergRestCatalogTest.java@156 PS10, Line 156: formatter.printHelp(USAGE_PREFIX, options); nit: we usually don't put multiple statements onto a single line as it is somewhat confusing. Same at L157, L161, L169. http://gerrit.cloudera.org:8080/#/c/22878/10/tests/common/iceberg_rest_server.py File tests/common/iceberg_rest_server.py: http://gerrit.cloudera.org:8080/#/c/22878/10/tests/common/iceberg_rest_server.py@61 PS10, Line 61: if self.stdout: : self.stdout.close() : if self.stderr: : self.stderr.close() I wonder if we should move it to a finally block, after the process is killed? E.g. it might log something when it receives the TERM signal. http://gerrit.cloudera.org:8080/#/c/22878/10/tests/custom_cluster/test_iceberg_rest_catalog.py File tests/custom_cluster/test_iceberg_rest_catalog.py: http://gerrit.cloudera.org:8080/#/c/22878/10/tests/custom_cluster/test_iceberg_rest_catalog.py@44 PS10, Line 44: def RestServerProperties(*server_configs): Btw, I like this decorator approach! http://gerrit.cloudera.org:8080/#/c/22878/10/tests/custom_cluster/test_iceberg_rest_catalog.py@161 PS10, Line 161: catalogd_args=MULTICATALOG_CATALOGD_ARGS We don't have catalogd, so we probably don't need this. Same goes for L174. -- To view, visit http://gerrit.cloudera.org:8080/22878 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifbdd0f7085345e7954d9f6f264202699182dd1e1 Gerrit-Change-Number: 22878 Gerrit-PatchSet: 10 Gerrit-Owner: Peter Rozsa <[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: Thu, 18 Sep 2025 17:28:16 +0000 Gerrit-HasComments: Yes
