Csaba Ringhofer 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: (8 comments) http://gerrit.cloudera.org:8080/#/c/11531/5/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/5/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@563 PS5, Line 563: if (filter != null) { : if (isPrivilegeFiltered(filter, privilege)) continue; : } This seems to be the same logic as line 584, so it would better to make them look consistent. http://gerrit.cloudera.org:8080/#/c/11531/5/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@583 PS5, Line 583: TPrivilege privilege = p.toThrift(); : if (filter != null && isPrivilegeFiltered(filter, privilege)) continue; : TResultRowBuilder rowBuilder = new TResultRowBuilder(); : rowBuilder.add(Strings.nullToEmpty(TPrincipalType.ROLE.name().toUpperCase())); : rowBuilder.add(Strings.nullToEmpty(rolePrincipal.getName())); : result.addToRows(addShowPrincipalOutputResults(privilege, rowBuilder).get()); optional: The block in this loop and the one at line 561 seems very similar and could be extracted to a like createShowUserPrivilegesResultRow() http://gerrit.cloudera.org:8080/#/c/11531/5/fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java File fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java: http://gerrit.cloudera.org:8080/#/c/11531/5/fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java@32 PS5, Line 32: CustomClusterGroupMapper I can't find where this class is used. http://gerrit.cloudera.org:8080/#/c/11531/5/fe/src/test/java/org/apache/impala/service/CustomClusterResourceAuthorizationProvider.java File fe/src/test/java/org/apache/impala/service/CustomClusterResourceAuthorizationProvider.java: http://gerrit.cloudera.org:8080/#/c/11531/5/fe/src/test/java/org/apache/impala/service/CustomClusterResourceAuthorizationProvider.java@25 PS5, Line 25: CustomClusterResourceAuthorizationProvider I can't find where this class is used. http://gerrit.cloudera.org:8080/#/c/11531/5/tests/authorization/test_owner_privileges.py File tests/authorization/test_owner_privileges.py: http://gerrit.cloudera.org:8080/#/c/11531/5/tests/authorization/test_owner_privileges.py@163 PS5, Line 163: ignore_role=True optional: ignore_role seems redundant to me - it is always true for "show grant user", and always false for "show grant role". I see several options, like deducing information by checking the query string, or creating separate validate_role_privileges/validate_user_privileges functions. http://gerrit.cloudera.org:8080/#/c/11531/5/tests/common/sentry_cache_test_suite.py File tests/common/sentry_cache_test_suite.py: http://gerrit.cloudera.org:8080/#/c/11531/5/tests/common/sentry_cache_test_suite.py@47 PS5, Line 47: If we ignore_role : output This part of the sentence seem strange. http://gerrit.cloudera.org:8080/#/c/11531/5/tests/common/sentry_cache_test_suite.py@62 PS5, Line 62: offset optional: offsets could be replace with if ignore_role: col = col[2:] http://gerrit.cloudera.org:8080/#/c/11531/5/tests/common/sentry_cache_test_suite.py@75 PS5, Line 75: the ignore role privileges. I do not understand this part of the sentence - can you rephrase it? -- 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: Csaba Ringhofer <csringho...@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 20:32:05 +0000 Gerrit-HasComments: Yes
