Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/23833 )
Change subject: IMPALA-14341: Iceberg MetaProviders Blacklist DBs/Tables ...................................................................... Patch Set 18: (15 comments) http://gerrit.cloudera.org:8080/#/c/23833/12/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/23833/12/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@579 PS12, Line 579: public boolean isBlacklistedTable(String db, String table) { : return isBlacklistedTable(new TableName(db, table)); : } : : public void setAuthzManager(AuthorizationManage > Any reason keeping this code commented out? Nope, it's gone now. http://gerrit.cloudera.org:8080/#/c/23833/12/fe/src/main/java/org/apache/impala/catalog/local/BlacklistingMetaProvider.java File fe/src/main/java/org/apache/impala/catalog/local/BlacklistingMetaProvider.java: http://gerrit.cloudera.org:8080/#/c/23833/12/fe/src/main/java/org/apache/impala/catalog/local/BlacklistingMetaProvider.java@51 PS12, Line 51: return super.loadDbList().stream().filter( > I wonder if it would make sense to introduce a class that delegates every m I like that idea since it would make it really easy to figure out what exactly the BlacklistingMetaProvider class was doing. Generally, I do not favor having abstract classes with a single subclass, but I would like to encourage using the decorator pattern in the future should a new use case similar to this one arise. http://gerrit.cloudera.org:8080/#/c/23833/12/fe/src/main/java/org/apache/impala/catalog/local/BlacklistingMetaProvider.java@58 PS12, Line 58: ount() = > nit: we usually put underscore after member names: delegate_ Done http://gerrit.cloudera.org:8080/#/c/23833/12/fe/src/main/java/org/apache/impala/catalog/local/BlacklistingMetaProvider.java@84 PS12, Line 84: > parallelStream() seems to be an overkill for this. I switched to stream(). I did some testing, and parallelStream() is faster at higher number of values (1,000,000), but when the value is 1,000, then stream() is faster. http://gerrit.cloudera.org:8080/#/c/23833/12/fe/src/main/java/org/apache/impala/catalog/local/BlacklistingMetaProvider.java@84 PS12, Line 84: > Maybe we could add a quick check: if there aren't any blacklisted DBs then Done http://gerrit.cloudera.org:8080/#/c/23833/12/fe/src/main/java/org/apache/impala/catalog/local/BlacklistingMetaProvider.java@89 PS12, Line 89: > Shouldn't we test if dbName is blacklisted? The loadDb method never gets called on a blacklisted db since the coordinator thinks that db does not even exist. http://gerrit.cloudera.org:8080/#/c/23833/12/fe/src/main/java/org/apache/impala/catalog/local/BlacklistingMetaProvider.java@94 PS12, Line 94: > parallelStream() seems to be an overkill for this. Done http://gerrit.cloudera.org:8080/#/c/23833/12/fe/src/main/java/org/apache/impala/catalog/local/BlacklistingMetaProvider.java@94 PS12, Line 94: > If we had a collection of the DBs with filtered tables, we could check if ' Good idea. Done http://gerrit.cloudera.org:8080/#/c/23833/12/fe/src/main/java/org/apache/impala/catalog/local/BlacklistingMetaProvider.java@101 PS12, Line 101: > Same as above, would it be safer to test if table is not blacklisted? Same Since blacklisted tables are not included in the results of loadTableList, this method is not called at all. There may be an exception in the future for 'create table' sqls when tables can be created via a REST catalog, but https://gerrit.cloudera.org/c/23833/14/testdata/workloads/functional-query/queries/QueryTest/iceberg-rest-catalog-blacklist-tables.test has a test case to guard against that situation. http://gerrit.cloudera.org:8080/#/c/23833/12/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/23833/12/fe/src/main/java/org/apache/impala/service/catalogmanager/LocalImpl.java@113 PS12, Line 113: IcebergMetaProvider > We caught a RESTException here which only comes from IcebergMetaProvider, s I think just having "IcebergMetaProvider" is the way to go simple because BlacklistingMetaProvider constructor doesn't throw exceptions, and including it in the error message will cause confusion. http://gerrit.cloudera.org:8080/#/c/23833/12/fe/src/test/java/org/apache/impala/catalog/local/BlacklistingMetaProviderTest.java File fe/src/test/java/org/apache/impala/catalog/local/BlacklistingMetaProviderTest.java: http://gerrit.cloudera.org:8080/#/c/23833/12/fe/src/test/java/org/apache/impala/catalog/local/BlacklistingMetaProviderTest.java@108 PS12, Line 108: > nit: indentation is off Done http://gerrit.cloudera.org:8080/#/c/23833/12/fe/src/test/java/org/apache/impala/util/CatalogBlacklistUtilsTest.java File fe/src/test/java/org/apache/impala/util/CatalogBlacklistUtilsTest.java: http://gerrit.cloudera.org:8080/#/c/23833/12/fe/src/test/java/org/apache/impala/util/CatalogBlacklistUtilsTest.java@133 PS12, Line 133: : > nit: one empty line is enough Done http://gerrit.cloudera.org:8080/#/c/23833/12/fe/src/test/java/org/apache/impala/util/CatalogBlacklistUtilsTest.java@156 PS12, Line 156: setBlacklist("", "db3.,.bar,,"); > nit: unnecessary empty line Done http://gerrit.cloudera.org:8080/#/c/23833/12/fe/src/test/java/org/apache/impala/util/CatalogBlacklistUtilsTest.java@163 PS12, Line 163: > nit: indentation is off Done http://gerrit.cloudera.org:8080/#/c/23833/12/testdata/workloads/functional-query/queries/QueryTest/iceberg-rest-catalog-blacklist-db.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-rest-catalog-blacklist-db.test: http://gerrit.cloudera.org:8080/#/c/23833/12/testdata/workloads/functional-query/queries/QueryTest/iceberg-rest-catalog-blacklist-db.test@14 PS12, Line 14: ==== > Can you add a test for trying to create a blacklisted database? Done. I could not find a way to force the create database to go to the REST catalog instead of Catalogd, but the test_rest_catalog_basic_blacklisted_db only spins up a REST catalog without any Catalogd. Therefore, at least one test attempts to create database against a REST catalog. -- To view, visit http://gerrit.cloudera.org:8080/23833 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I73a06d30dcdbb579f92c2afa5928beb6c5a13348 Gerrit-Change-Number: 23833 Gerrit-PatchSet: 18 Gerrit-Owner: Jason Fehr <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Vanko <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Mihaly Szjatinya <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Peter Rozsa <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Tue, 03 Feb 2026 20:36:14 +0000 Gerrit-HasComments: Yes
