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 4: (10 comments) http://gerrit.cloudera.org:8080/#/c/14134/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14134/4//COMMIT_MSG@9 PS4, Line 9: Add catalogd startup option for database and table blacklist. Blacklist Can we also add them to the coordinator startup flags and intercept any references to them in the analysis pass? (we have code for validating table and db names, so we can include this check too probably) For example: throw an AnalysisException that user cannot create tables with names in the blacklisted set? http://gerrit.cloudera.org:8080/#/c/14134/4/be/src/common/global-flags.cc File be/src/common/global-flags.cc: http://gerrit.cloudera.org:8080/#/c/14134/4/be/src/common/global-flags.cc@253 PS4, Line 253: nit: blacklisted (multiple places) http://gerrit.cloudera.org:8080/#/c/14134/4/be/src/common/global-flags.cc@256 PS4, Line 256: "Comma separated list for full names of blacklist tables. Configure which tables to " nit: Define the format to be clear? <db>.<tbl> ? http://gerrit.cloudera.org:8080/#/c/14134/4/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/14134/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@274 PS4, Line 274: protected final Map<String, Set<String>> blacklistTables_ = Maps.newHashMap(); nit: you can also do a Set<TableName> http://gerrit.cloudera.org:8080/#/c/14134/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@290 PS4, Line 290: blacklistDbs_, blacklistTables_); why pass class members? I'm guessing it is for unit tests, mention that? http://gerrit.cloudera.org:8080/#/c/14134/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@332 PS4, Line 332: public static void loadBlacklist(String blacklistDbsConfig, nit: call parseBlackListedDbsAndTbls() or something similar? http://gerrit.cloudera.org:8080/#/c/14134/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@335 PS4, Line 335: for (String db: blacklistDbsConfig.trim().split(",\\s*")) { nit: Splitter is a neater way of doing the same thing. It has helpers to trim, omitEmptyStrings etc.. https://guava.dev/releases/23.0/api/docs/com/google/common/base/Splitter.html http://gerrit.cloudera.org:8080/#/c/14134/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@338 PS4, Line 338: if (LOG.isTraceEnabled()) { nit: I think this can be info, since this is once per process lifetime and is critical piece of information. http://gerrit.cloudera.org:8080/#/c/14134/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@356 PS4, Line 356: continue; worthy of logging? http://gerrit.cloudera.org:8080/#/c/14134/4/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2121 PS4, Line 2121: return null; Like I mentioned in the commit message, I think we should incorporate checks in the analysis pass to prevent analysis on any table ref / db matching blacklists. Thoughts? -- 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: 4 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: Mon, 26 Aug 2019 16:44:53 +0000 Gerrit-HasComments: Yes
