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 8: (5 comments) lgtm barring some code refactoring suggestions. I think the issue with sys and information_schema not appearing is probably something to do with the version of hive pinned in the toolchain. Unfortunately we have to complicate the tests because of that. I think that shouldn't be a blocker given we have custom cluster tests for it and we can always fix them in the follow up commits. http://gerrit.cloudera.org:8080/#/c/14134/8/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/8/fe/src/main/java/org/apache/impala/analysis/TableName.java@86 PS8, Line 86: throw new AnalysisException("Invalid table/view name: " + tbl_); Is there a reason not to fold the Blacklist.verifyTableName() into TableName#analyze()? In other words, are there places where we want to skip the blacklist check for whatever reason? http://gerrit.cloudera.org:8080/#/c/14134/8/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/8/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@270 PS8, Line 270: // Error string for inconsistent blacklisted dbs/tables configs between catalogd and Yea, we don't have any checks that validate the configs across roles. That is likely to be a bigger change. http://gerrit.cloudera.org:8080/#/c/14134/8/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1519 PS8, Line 1519: addSummary(resp, "Can't drop blacklisted database: " + dbName); Add a test for this? http://gerrit.cloudera.org:8080/#/c/14134/8/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1644 PS8, Line 1644: Similar check here? http://gerrit.cloudera.org:8080/#/c/14134/8/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/8/fe/src/main/java/org/apache/impala/util/BlacklistUtils.java@32 PS8, Line 32: BlacklistUtils nit: Call this CatalogUtils or something? BlackList seems out of place, does not convey what is being blacklisted. -- 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: 8 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, 03 Sep 2019 21:57:02 +0000 Gerrit-HasComments: Yes
