Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11531 )
Change subject: IMPALA-7503: SHOW GRANT USER not showing all privileges. ...................................................................... Patch Set 5: (16 comments) http://gerrit.cloudera.org:8080/#/c/11531/4/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/11531/4/fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java@69 PS4, Line 69: switch (principalType_) { : case ROLE: : throw new AnalysisException(String.format("%s '%s' does not exist.", : Principal.toString(principalType_), name_)); : case USER: : // Create a user object here because it's possible the user does not exist in : // Sentry, but still exists according to the OS, or Hadoop, or other custom : // group mapping provider. : principal_ = Principal.newInstance(name_, principalType_, Sets.newHashSet()); : break; : default: : > Usually we use switch statement for enums: https://stackoverflow.com/questi Done http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java File fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java: http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@404 PS4, Line 404: } > nit: extra empty line Done http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@468 PS4, Line 468: * Allows for filtering based on a specific privilege spec or showing all privileges : * granted to the role. Used by the SHOW GRANT ROLE statement. : */ > replace the word "principal" to "role" Done http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@473 PS4, Line 473: ew TResultSet(); > Is the TPrincipalType still necessary since we know it should always be TPr Done http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@480 PS4, Line 480: ege p : role.getPrivileges()) { > Can we do getUser(principalName) instead? Done http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@485 PS4, Line 485: > use && with L484 to avoid nested ifs. Done http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@521 PS4, Line 521: ullToEmpty(privilege.g > Maybe addShowPrincipalOutputResults is a better name? addCommonOutputResult Done http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@542 PS4, Line 542: t = new T > replace principal with user. Done http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@545 PS4, Line 545: dToColumns(new TCol > Is the TPrincipalType necessary since we know it should always be TPrincipa Done http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@557 PS4, Line 557: cipalName > user Done http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@585 PS4, Line 585: TResultRowBuilder rowBuilder = new TResultRowBuilder(); > Can we do getRole(role.getName()) instead? Done http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@590 PS4, Line 590: > use && with L589 to avoid nested ifs. Done http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/service/JniFrontend.java File fe/src/main/java/org/apache/impala/service/JniFrontend.java: http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/service/JniFrontend.java@564 PS4, Line 564: switch (params.getPrincipal_type()) { : case USER: : result = frontend_.getCatalog().getAuthPolicy().getUserPrivileges( : params.getName(), params.getPrivilege(), frontend_); : break; : case ROLE: : result = frontend_.getCatalog().getAuthPolicy().getRolePrivileges( : params.getName(), params.getPrivilege()); : break; : > Usually we use switch statement for enums: https://stackoverflow.com/questi Done http://gerrit.cloudera.org:8080/#/c/11531/4/tests/authorization/test_owner_privileges.py File tests/authorization/test_owner_privileges.py: http://gerrit.cloudera.org:8080/#/c/11531/4/tests/authorization/test_owner_privileges.py@89 PS4, Line 89: principal_name, principal_type, scope, database, table, column, uri, priv > Update comment to include missing "principal_type, principal_name" Done http://gerrit.cloudera.org:8080/#/c/11531/2/tests/authorization/test_show_grant_user.py File tests/authorization/test_show_grant_user.py: http://gerrit.cloudera.org:8080/#/c/11531/2/tests/authorization/test_show_grant_user.py@75 PS2, Line 75: def restart_first_impalad(cls): : impala > I don't think that's what try/finally is for. Done http://gerrit.cloudera.org:8080/#/c/11531/4/tests/authorization/test_show_grant_user.py File tests/authorization/test_show_grant_user.py: http://gerrit.cloudera.org:8080/#/c/11531/4/tests/authorization/test_show_grant_user.py@86 PS4, Line 86: 'org.apache.impala.service.CustomClusterResourceAuthorizationProvider' : .format(SENTRY_CONFIG_FILE), : sentry_config=SENTRY_CONFIG_FILE) : def test_show_grant_user(self, vector, unique_database): : group_name = grp.getgrnam(getuser()).gr_name : self.client.execute('create role sgu_test_primary') : self.client.execute('grant all on s > A mixed use of + and Python string format. Can we use Python string format Done -- To view, visit http://gerrit.cloudera.org:8080/11531 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie9f6c88f5569e1c414ceb8a86e7b013eaa3ecde1 Gerrit-Change-Number: 11531 Gerrit-PatchSet: 5 Gerrit-Owner: Adam Holley <ahol...@cloudera.com> Gerrit-Reviewer: Adam Holley <ahol...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Thu, 27 Sep 2018 18:35:15 +0000 Gerrit-HasComments: Yes
