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 6: (10 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@478 PS5, Line 478: Role role > nit: this should be Role Done http://gerrit.cloudera.org:8080/#/c/11531/5/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@559 PS5, Line 559: User user > nit: this should be User Done http://gerrit.cloudera.org:8080/#/c/11531/5/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@563 PS5, Line 563: } : : // Ge > This seems to be the same logic as line 584, so it would better to make the Done http://gerrit.cloudera.org:8080/#/c/11531/5/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@583 PS5, Line 583: * and associated roles. : */ : private void createShowUserPrivilegesResultRows(TResultSet result, : List<PrincipalPrivilege> privileges, TPrivilege filter, String name, : TPrincipalType type) { : for (PrincipalPrivilege p : privileges) { > optional: The block in this loop and the one at line 561 seems very similar Done 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. It's used in test_show_grant_user.py via the CustomClusterResourceAuthorizationProvider 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. It's used in test_show_grant_user.py. 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: > optional: ignore_role seems redundant to me - it is always true for "show g Done 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 the query is : for "s > This part of the sentence seem strange. Reworded. http://gerrit.cloudera.org:8080/#/c/11531/5/tests/common/sentry_cache_test_suite.py@62 PS5, Line 62: [2:] > optional: offsets could be replace with Done http://gerrit.cloudera.org:8080/#/c/11531/5/tests/common/sentry_cache_test_suite.py@75 PS5, Line 75: ached. If delay_s is > 0 th > I do not understand this part of the sentence - can you rephrase it? Removed. -- 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: 6 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 21:23:40 +0000 Gerrit-HasComments: Yes
