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 5: (8 comments) http://gerrit.cloudera.org:8080/#/c/22878/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22878/2//COMMIT_MSG@13 PS2, Line 13: In database listings, if one database name occurs multiple times the : contained tables are merged under that database n > What happens if there are two databases with the same name, but the table n Done http://gerrit.cloudera.org:8080/#/c/22878/2/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/2/fe/src/main/java/org/apache/impala/catalog/local/MultiMetaProvider.java@106 PS2, Line 106: > Why do we need 'this.'? Done http://gerrit.cloudera.org:8080/#/c/22878/2/fe/src/main/java/org/apache/impala/catalog/local/MultiMetaProvider.java@105 PS2, Line 105: : @Override : public Database loadDb(String dbName) throws TException { : return tryAllProviders( : unchecked(provider -> provider.loadDb(dbName))); : } > This code structure repeats quite a few times. Can this be moved to a commo Done http://gerrit.cloudera.org:8080/#/c/22878/2/fe/src/main/java/org/apache/impala/catalog/local/MultiMetaProvider.java@280 PS2, Line 280: } > This is a nice method, but most of the time we should know which metaprovid As we discussed offline, the exception messages are now collected and only reported when all providers are failed. http://gerrit.cloudera.org:8080/#/c/22878/2/fe/src/main/java/org/apache/impala/catalog/local/MultiMetaProvider.java@310 PS2, Line 310: }; > Please add comment for this class. Done http://gerrit.cloudera.org:8080/#/c/22878/2/fe/src/main/java/org/apache/impala/service/catalogmanager/CatalogdImpl.java File fe/src/main/java/org/apache/impala/service/catalogmanager/CatalogdImpl.java: http://gerrit.cloudera.org:8080/#/c/22878/2/fe/src/main/java/org/apache/impala/service/catalogmanager/CatalogdImpl.java@49 PS2, Line 49: r > nit: single-line if statements don't need braces. Same in L63. Done http://gerrit.cloudera.org:8080/#/c/22878/2/fe/src/main/java/org/apache/impala/service/catalogmanager/LocalImpl.java File fe/src/main/java/org/apache/impala/service/catalogmanager/LocalImpl.java: http://gerrit.cloudera.org:8080/#/c/22878/2/fe/src/main/java/org/apache/impala/service/catalogmanager/LocalImpl.java@111 PS2, Line 111: > Don't you need another {} for 'e'? According to https://www.slf4j.org/api/o Done http://gerrit.cloudera.org:8080/#/c/22878/2/tests/custom_cluster/test_iceberg_rest_catalog.py File tests/custom_cluster/test_iceberg_rest_catalog.py: http://gerrit.cloudera.org:8080/#/c/22878/2/tests/custom_cluster/test_iceberg_rest_catalog.py@94 PS2, Line 94: self.run_test_case('QueryTest/iceberg-rest-catalog', vector, use_db="ice") > We could test: 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: 5 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: Tue, 29 Jul 2025 08:50:03 +0000 Gerrit-HasComments: Yes
