Fredy Wijaya 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 4: (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: if (principalType_ == TPrincipalType.ROLE) { : throw new AnalysisException(String.format("%s '%s' does not exist.", : Principal.toString(principalType_), name_)); : } else if (principalType_ == TPrincipalType.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()); : } else { : throw new AnalysisException(String.format("Unexpected TPrincipalType: %s", : Principal.toString(principalType_))); : } Usually we use switch statement for enums: https://stackoverflow.com/questions/4922932/java-performance-of-enums-vs-if-then-else 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 http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@468 PS4, Line 468: * Returns the privileges that have been granted to a principal as a tabular result set. : * Allows for filtering based on a specific privilege spec or showing all privileges : * granted to the principal. Used by the SHOW GRANT ROLE statement. replace the word "principal" to "role" http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@473 PS4, Line 473: TPrincipalType type Is the TPrincipalType still necessary since we know it should always be TPrincipalType.USER? http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@480 PS4, Line 480: getPrincipal(principalName, type) Can we do getUser(principalName) instead? http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@485 PS4, Line 485: if (isPrivilegeFiltered(filter, privilege)) continue; use && with L484 to avoid nested ifs. http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@521 PS4, Line 521: addCommonOutputResults Maybe addShowPrincipalOutputResults is a better name? addCommonOutputResults is to generic and this class is not specific to "SHOW GRANT" commands. http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@542 PS4, Line 542: principal replace principal with user. http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@545 PS4, Line 545: TPrincipalType type Is the TPrincipalType necessary since we know it should always be TPrincipalType.USER? http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@557 PS4, Line 557: principal user http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@585 PS4, Line 585: Principal rolePrincipal = getPrincipal(role.getName(), TPrincipalType.ROLE); Can we do getRole(role.getName()) instead? http://gerrit.cloudera.org:8080/#/c/11531/4/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@590 PS4, Line 590: if (isPrivilegeFiltered(filter, privilege)) continue; use && with L589 to avoid nested ifs. 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: if (params.getPrincipal_type() == TPrincipalType.USER) { : result = frontend_.getCatalog().getAuthPolicy().getUserPrivileges( : params.getName(), params.getPrivilege(), params.getPrincipal_type(), frontend_); : } else if (params.getPrincipal_type() == TPrincipalType.ROLE) { : result = frontend_.getCatalog().getAuthPolicy().getRolePrivileges( : params.getName(), params.getPrivilege(), params.getPrincipal_type()); : } else { : throw new AnalysisException("Unexpected TPrincipalType: " + : params.getPrincipal_type()); : } Usually we use switch statement for enums: https://stackoverflow.com/questions/4922932/java-performance-of-enums-vs-if-then-else 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: scope, database, table, column, uri, privilege, grant_option, create_time Update comment to include missing "principal_type, principal_name" 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: # Ignore any errors here because these might fail for partial test runs. : pass > I need an finally or except to ignore the statements above that might fail I don't think that's what try/finally is for. 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: impalad_args='--server_name=server1 --authorization_policy_provider_class=' + : 'org.apache.impala.service.CustomClusterResourceAuthorizationProvider ' + : '--sentry_config={0}'.format(SENTRY_CONFIG_FILE), : catalogd_args='--sentry_config={0}'.format(SENTRY_CONFIG_FILE) + : ' --authorization_policy_provider_class=' + : 'org.apache.impala.service.CustomClusterResourceAuthorizationProvider', : sentry_config=SENTRY_CONFIG_FILE) A mixed use of + and Python string format. Can we use Python string format instead? -- 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: 4 Gerrit-Owner: Adam Holley <[email protected]> Gerrit-Reviewer: Adam Holley <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Thu, 27 Sep 2018 18:10:25 +0000 Gerrit-HasComments: Yes
