Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/14134 )
Change subject: IMPALA-8797: Support database and table blacklist ...................................................................... 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). 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 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? (same below) 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. 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#isValidDbName() or something like that. 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... ? 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 blacklisted using --black_...." or something like that. 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 multiple places? 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? 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? 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? 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 checks are in place, we'd never pass blacklisted db/tables until this point. 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) 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) -- 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: Tue, 27 Aug 2019 19:15:57 +0000 Gerrit-HasComments: Yes
