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 8: (5 comments) > 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. I've tried USE_CDP_HIVE=true to use Hive3, but there are still no "sys" and "information_schema" databases. Maybe it requires some configuration change or it's not shown for postgresql. Hope I can find the cause tomorrow. 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 TableNam I think they should always be called together. But BlacklistUtils.verifyTableName() requires a fully-qualified TableName object. I used to add it into TableName.analyze() in a previous patch version, then the codes like this: tableName_.analyze(); become something like: analyzer.getFqTableName(tableName_).analyze(); Since "analyze" sounds like changing the state (to be analyzed) of the target. Now we look like changing the state of a temp object... So I decided to refactor the function name to "verify", meaning we are just verifying something based on the temp object. Finally, we decided to keep the name as "analyze" to be consistent with other parts (in the review of patch set 7). So I moved out the logics of BlacklistUtils.verifyTableName() since it requires more (valid db_) comparing to TableName.analyze(). However, I'm ok if we should merge them together and writing codes like analyzer.getFqTableName(tableName_).analyze(); 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 Yes, I think for configs that read by both catalogd and coordinators, we should always check the consistent. I used to just give the configs to catalogd and found wired behaviors. So I decide to add these checks. 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? This is tested in tests/custom_cluster/test_blacklisted_dbs_and_tables.py: self.__expect_error_in_result( "drop database if exists functional_rc", "Can't drop blacklisted database: functional_rc") Note that it uses __expect_error_in_result, not __expect_error_in_query. 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? Good point! I missed it since the bug of IMPALA-8918. Will add more checks. 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, doe Sure. What about CatalogBlacklistUtils? -- 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: Wed, 04 Sep 2019 13:55:04 +0000 Gerrit-HasComments: Yes
