Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/14134 )
Change subject: IMPALA-8797: Support database and table blacklist ...................................................................... Patch Set 7: (14 comments) > Patch Set 7: > > (13 comments) > > Couple of requests for test coverage. > > - Add more fe unit tests on default sys/information schema? (one can't > create/alter/access them). > - Add a GET_SCHEMAS/DBs test that it doesn't return these databases by > default (this is the source of this enhancement request, so just want to be > sure there is some coverage for that). I'd like to add these. But Hive3 in the minicluster doesn't show the sys and information_schema databases. I'm still finding the right configs for this. http://gerrit.cloudera.org:8080/#/c/14134/7/be/src/common/global-flags.cc File be/src/common/global-flags.cc: http://gerrit.cloudera.org:8080/#/c/14134/7/be/src/common/global-flags.cc@253 PS7, Line 253: blacklist > nit:blacklisted Done http://gerrit.cloudera.org:8080/#/c/14134/7/be/src/common/global-flags.cc@254 PS7, Line 254: Users don't see them when querying the database list. > nit:Instead mention that users cannot query or access these databases? (sam Done http://gerrit.cloudera.org:8080/#/c/14134/7/be/src/common/global-flags.cc@253 PS7, Line 253: Configure which databases to be " : "skipped for loading > nit: I think this is obvious, can be skipped. I think without this, users may think the blacklisted databases are still visible but just can't be used. So may still worth to mention it. http://gerrit.cloudera.org:8080/#/c/14134/7/fe/src/main/java/org/apache/impala/analysis/CreateDbStmt.java File fe/src/main/java/org/apache/impala/analysis/CreateDbStmt.java: http://gerrit.cloudera.org:8080/#/c/14134/7/fe/src/main/java/org/apache/impala/analysis/CreateDbStmt.java@36 PS7, Line 36: private final static Set<String> BLACKLISTED_DBS = BlacklistUtils.parseBlacklistedDbs( > Also consider moving this to Analyzer and you can have a helper Analyzer#is Good point! To avoid adding everything into the Analyzer with the fact that it's super huge now, I think we can add these static logics into BlacklistUtils. http://gerrit.cloudera.org:8080/#/c/14134/7/fe/src/main/java/org/apache/impala/analysis/CreateDbStmt.java@37 PS7, Line 37: BackendConfig.INSTANCE == null ? "" : BackendConfig.INSTANCE.getBlacklistedDbs(), : null > fold this logic into parseBlack... ? Done http://gerrit.cloudera.org:8080/#/c/14134/7/fe/src/main/java/org/apache/impala/analysis/CreateDbStmt.java@102 PS7, Line 102: Can't use blacklisted database name: " + dbName_ > Keep the message consistent with above? "Invalid database name. It has been Done http://gerrit.cloudera.org:8080/#/c/14134/7/fe/src/main/java/org/apache/impala/analysis/TableName.java File fe/src/main/java/org/apache/impala/analysis/TableName.java: http://gerrit.cloudera.org:8080/#/c/14134/7/fe/src/main/java/org/apache/impala/analysis/TableName.java@42 PS7, Line 42: private final static Set<TableName> BLACKLISTED_TABLES = > Consider putting a copy of this in Analyzer rather than using it in multipl Done http://gerrit.cloudera.org:8080/#/c/14134/7/fe/src/main/java/org/apache/impala/analysis/TableName.java@87 PS7, Line 87: verify > I think the code generally calls it "analyze()". Keep it consistent? Reverted the changes in this function since we need the analyzer. http://gerrit.cloudera.org:8080/#/c/14134/7/fe/src/main/java/org/apache/impala/analysis/TableName.java@90 PS7, Line 90: throw new AnalysisException("Invalid database name: " + db_); > Do we need to check if the db_ is not black listed? I think we don't need it. Blacklisted databases are not found in the catalog cache. It will fail by DatabaseNotFoundException thrown later in https://github.com/apache/impala/blob/3f4cbe961702552a187c0bbc04b616e0029c1412/fe/src/main/java/org/apache/impala/analysis/TableDef.java#L230 http://gerrit.cloudera.org:8080/#/c/14134/7/fe/src/main/java/org/apache/impala/analysis/TableName.java@97 PS7, Line 97: Can't use blacklisted table nam > nit: Keep it consistent with above? Done http://gerrit.cloudera.org:8080/#/c/14134/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/14134/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@70 PS7, Line 70: import org.apache.impala.catalog.DatabaseNotFoundException; > Convert all the checks in here to Preconditions? If all the right Analyzer Sure. Good point! http://gerrit.cloudera.org:8080/#/c/14134/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3709 PS7, Line 3709: if (catalog_.isBlacklistedDb(req.getDb_name())) { Remove these since we can't reach here: blacklisted dbs in catalogd won't propagate to coordinators. So REFRESH FUNCTIONS for blacklisted dbs will encounter DatabaseNotFoundException in the coordinator. http://gerrit.cloudera.org:8080/#/c/14134/7/fe/src/main/java/org/apache/impala/util/BlacklistUtils.java File fe/src/main/java/org/apache/impala/util/BlacklistUtils.java: http://gerrit.cloudera.org:8080/#/c/14134/7/fe/src/main/java/org/apache/impala/util/BlacklistUtils.java@22 PS7, Line 22: blacklistedDbsConfig > nit: checkNotNull()? (same below) Done http://gerrit.cloudera.org:8080/#/c/14134/7/tests/custom_cluster/test_blacklisted_dbs_and_tables.py File tests/custom_cluster/test_blacklisted_dbs_and_tables.py: http://gerrit.cloudera.org:8080/#/c/14134/7/tests/custom_cluster/test_blacklisted_dbs_and_tables.py@46 PS7, Line 46: visable > typo (multiple places) Done -- To view, visit http://gerrit.cloudera.org:8080/14134 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I02dbb07f8e08793b57b2a88d09b30fd32cff26dc Gerrit-Change-Number: 14134 Gerrit-PatchSet: 7 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Thu, 29 Aug 2019 14:47:04 +0000 Gerrit-HasComments: Yes
