[Impala-ASF-CR] IMPALA-8797: Support database and table blacklist

["Bharath Vissapragada (Code Review)" <ge...@cloudera.org>]

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 <hu...@gmail.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Comment-Date: Tue, 03 Sep 2019 21:57:02 +0000
Gerrit-HasComments: Yes