Peter Rozsa 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 8: (10 comments) http://gerrit.cloudera.org:8080/#/c/22878/7/fe/src/main/java/org/apache/impala/catalog/local/MultiMetaProvider.java File fe/src/main/java/org/apache/impala/catalog/local/MultiMetaProvider.java: http://gerrit.cloudera.org:8080/#/c/22878/7/fe/src/main/java/org/apache/impala/catalog/local/MultiMetaProvider.java@102 PS7, Line 102: loadDbList)).stream().flatMap( > Can we replace it with 'distinct()'? Done http://gerrit.cloudera.org:8080/#/c/22878/7/fe/src/main/java/org/apache/impala/catalog/local/MultiMetaProvider.java@118 PS7, Line 118: first > nit: please rename to 'firstDuplicate' to make it more verbose Done http://gerrit.cloudera.org:8080/#/c/22878/7/fe/src/main/java/org/apache/impala/catalog/local/MultiMetaProvider.java@122 PS7, Line 122: ( > nit: extra space Done http://gerrit.cloudera.org:8080/#/c/22878/7/fe/src/main/java/org/apache/impala/catalog/local/MultiMetaProvider.java@123 PS7, Line 123: Ambiguous table name > Can we test it? Probably it's enough to just sping up multiple REST catalog Done http://gerrit.cloudera.org:8080/#/c/22878/7/fe/src/main/java/org/apache/impala/catalog/local/MultiMetaProvider.java@185 PS7, Line 185: rovider -> p > Do we need 'Metaprovider' here? Above we just have 'provider -> provider.lo Done http://gerrit.cloudera.org:8080/#/c/22878/7/fe/src/main/java/org/apache/impala/catalog/local/MultiMetaProvider.java@243 PS7, Line 243: nction.Function<MetaProvider, R> function) : throws TException { : Map<String, Exception> exceptions = new H > nit: could be extracted to a method "getAllProviders()". This could be also Done http://gerrit.cloudera.org:8080/#/c/22878/7/fe/src/main/java/org/apache/impala/service/catalogmanager/FeCatalogManager.java File fe/src/main/java/org/apache/impala/service/catalogmanager/FeCatalogManager.java: http://gerrit.cloudera.org:8080/#/c/22878/7/fe/src/main/java/org/apache/impala/service/catalogmanager/FeCatalogManager.java@50 PS7, Line 50: re > nit: too much indent Done http://gerrit.cloudera.org:8080/#/c/22878/7/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/7/java/iceberg-rest-catalog-test/src/main/java/org/apache/iceberg/rest/IcebergRestCatalogTest.java@126 PS7, Line 126: : public void stop() throws Exception { : if (httpServer != null) { : httpServer.stop(); : LOG.info("Iceberg REST Catalog stopped."); : } : } : : public static void main(String[] args) throws Exception { : Options options = new Options(); : : options.addOption(new Option(null, PORT_LONGOPT, true, : "Port for the REST catalog server (default: " + DEFAULT_REST_PORT + ")")); : : options.addOption(new Option(null, CATALOG_LOCATION_LONGOPT, true, : "Base location for the Hadoop catalog (default: " : + DEFAULT_HADOOP_CATALOG_LOCATION + ")")); : : options.addOption(new Option(null, HELP_LONGOPT, false, "Display this help message")); : : CommandLineParser parser = new BasicParser(); : HelpFormatter formatter = new HelpFormatter(); : CommandLine cmd; : : int port = DEFAULT_REST_PORT; : String catalogLocation = DEFAULT_HADOOP_CATALOG_LOCATION; : : try { : cmd = parser.parse(options, args); : } catch (ParseException e) { : LOG.error("Error: {}", e.getMessage()); formatter.printHelp(USAGE_PREFIX, options); : System.exit(1); return; : } : : if (cmd.hasOption(HELP_LONGOPT)) { : formatter.printHelp(USAGE_PREFIX, options); System.exit(0); : } : : if (cmd.hasOption(PORT_LONGOPT)) { : try { : port = Integer.parseInt(cmd.getOptionValue(PORT_LONGOPT)); : } catch (NumberFormatException e) { : LOG.error("Error: --port requires a valid integer value. Got: {}", : cmd.getOptionValue(PORT_LONGOPT)); formatter.printHelp(USAGE_PREFIX, options); : System.exit(1); : } : } : : if (cmd.hasOption(CATALOG_LOCATION_LONGOPT)) { : catalogLocation = cmd.getOptionValue(CATALOG_LOCATION_LONGOPT); : } : : > optional: I think it would be shorter and more readable with apache's commo I would like to skip adding another dependency to the REST catalog's pom, but it's reasonable to use a library here, also it's already included in the main project's pom. http://gerrit.cloudera.org:8080/#/c/22878/7/testdata/workloads/functional-query/queries/QueryTest/iceberg-multiple-rest-catalogs.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-multiple-rest-catalogs.test: http://gerrit.cloudera.org:8080/#/c/22878/7/testdata/workloads/functional-query/queries/QueryTest/iceberg-multiple-rest-catalogs.test@27 PS7, Line 27: SELECT ice_air.iata, ice_air.lat, berg_air.lon > nit: for readability, please use multiple lines Done http://gerrit.cloudera.org:8080/#/c/22878/7/tests/custom_cluster/test_iceberg_rest_catalog.py File tests/custom_cluster/test_iceberg_rest_catalog.py: http://gerrit.cloudera.org:8080/#/c/22878/7/tests/custom_cluster/test_iceberg_rest_catalog.py@69 PS7, Line 69: > nit: continuation lines need +4 indent Done -- 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: 8 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: Wed, 17 Sep 2025 08:04:43 +0000 Gerrit-HasComments: Yes
