Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/13074 )
Change subject: [WIP] IMPALA-8281: Add support for show grant user/group with Ranger ...................................................................... Patch Set 6: (8 comments) First pass on this. Will take a look at it more. http://gerrit.cloudera.org:8080/#/c/13074/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13074/6//COMMIT_MSG@11 PS6, Line 11: is called from impalad. Mention the new syntax and the syntax that's not supported by Ranger, such as the one without the on clause. http://gerrit.cloudera.org:8080/#/c/13074/6/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/13074/6/fe/src/main/cup/sql-parser.cup@968 PS6, Line 968: col_name.getTableName(), : Collections.singletonList(col_name.getColumnName()))) nit: indent more because it's part of PrivilegeSpec.createColumnScopedPriv http://gerrit.cloudera.org:8080/#/c/13074/6/fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java File fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java: http://gerrit.cloudera.org:8080/#/c/13074/6/fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java@63 PS6, Line 63: principal_ = analyzer.getCatalog().getAuthPolicy().getPrincipal(name_, : principalType_); : if (principal_ == null) throw new AnalysisException(String.format("%s '%s' " + : "does not exist.", Principal.toString(principalType_), name_)); : break; we should move this logic to Sentry specific implementation http://gerrit.cloudera.org:8080/#/c/13074/6/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java: http://gerrit.cloudera.org:8080/#/c/13074/6/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java@183 PS6, Line 183: return null; shouldn't this throw an exception instead? http://gerrit.cloudera.org:8080/#/c/13074/6/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java: http://gerrit.cloudera.org:8080/#/c/13074/6/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java@191 PS6, Line 191: } need default case or else the compiler may give a warning http://gerrit.cloudera.org:8080/#/c/13074/6/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java@354 PS6, Line 354: Unsupported principal type. mention the unsupported type name http://gerrit.cloudera.org:8080/#/c/13074/6/fe/src/main/java/org/apache/impala/catalog/Principal.java File fe/src/main/java/org/apache/impala/catalog/Principal.java: http://gerrit.cloudera.org:8080/#/c/13074/6/fe/src/main/java/org/apache/impala/catalog/Principal.java@183 PS6, Line 183: principal = ""; shouldn't this be an exception? http://gerrit.cloudera.org:8080/#/c/13074/6/tests/authorization/test_ranger.py File tests/authorization/test_ranger.py: http://gerrit.cloudera.org:8080/#/c/13074/6/tests/authorization/test_ranger.py@87 PS6, Line 87: result = self.execute_query("show grant {0} {1} on database {2}" This is good that we can now replace with this with "show grant". However, we need to write more exhaustive tests that does "show grant" in every single scope. We should also test "show grant" without "on" is not supported in Ranger. -- To view, visit http://gerrit.cloudera.org:8080/13074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic46fb9fc36c9e11ec78d5840d22eb0668150c2a4 Gerrit-Change-Number: 13074 Gerrit-PatchSet: 6 Gerrit-Owner: Austin Nobis <ano...@cloudera.com> Gerrit-Reviewer: Austin Nobis <ano...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Wed, 24 Apr 2019 22:56:17 +0000 Gerrit-HasComments: Yes
