Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/23833 )
Change subject: IMPALA-14341: Iceberg MetaProviders Blacklist DBs/Tables ...................................................................... Patch Set 12: (12 comments) Thanks for working on this! 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: // if (table.getDb().equalsIgnoreCase(Db.SYS) && blacklistedDbs_.contains(Db.SYS)) { : // // If we've overridden the database blacklist, only allow Impala system tables. : // return !impalaSysTables.contains(table.getTbl()); : // } : // return blacklistedTables_.contains(table); Any reason keeping this code commented out? 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: A MetaProvider that wraps another MetaProvider I wonder if it would make sense to introduce a class that delegates every method. We could write a Unit test that use reflection to check if it overrides every method from MetaProvider. Then this class would just need to override a few methods. http://gerrit.cloudera.org:8080/#/c/23833/12/fe/src/main/java/org/apache/impala/catalog/local/BlacklistingMetaProvider.java@58 PS12, Line 58: delegate nit: we usually put underscore after member names: delegate_ http://gerrit.cloudera.org:8080/#/c/23833/12/fe/src/main/java/org/apache/impala/catalog/local/BlacklistingMetaProvider.java@84 PS12, Line 84: .parallelStream() parallelStream() seems to be an overkill for this. http://gerrit.cloudera.org:8080/#/c/23833/12/fe/src/main/java/org/apache/impala/catalog/local/BlacklistingMetaProvider.java@89 PS12, Line 89: return this.delegate.loadDb(dbName); Shouldn't we test if dbName is blacklisted? http://gerrit.cloudera.org:8080/#/c/23833/12/fe/src/main/java/org/apache/impala/catalog/local/BlacklistingMetaProvider.java@94 PS12, Line 94: .parallelStream() parallelStream() seems to be an overkill for this. http://gerrit.cloudera.org:8080/#/c/23833/12/fe/src/main/java/org/apache/impala/catalog/local/BlacklistingMetaProvider.java@101 PS12, Line 101: return this.delegate.loadTable(dbName, tableName); Same as above, would it be safer to test if table is not blacklisted? Same goes for getTableIfPresent() 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 > update to "BlacklistingMetaProvider or IcebergMetaProvider"? We caught a RESTException here which only comes from IcebergMetaProvider, so I think having IcebergMetaProvider here is correct. 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 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 http://gerrit.cloudera.org:8080/#/c/23833/12/fe/src/test/java/org/apache/impala/util/CatalogBlacklistUtilsTest.java@156 PS12, Line 156: nit: unnecessary empty line 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 -- 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: 12 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, 20 Jan 2026 18:56:07 +0000 Gerrit-HasComments: Yes
