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 3: (19 comments) http://gerrit.cloudera.org:8080/#/c/11531/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11531/2//COMMIT_MSG@11 PS2, Line 11: The output for SHOW : GRANT USER will have two additional columns for privilege name and : privilege type so the user can know where the privilege comes from. : > Can we show a sample output here? Done http://gerrit.cloudera.org:8080/#/c/11531/2/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/2/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@471 PS2, Line 471: */ > I feel like it's better to have two separate methods, getRolePrivileges() a Done http://gerrit.cloudera.org:8080/#/c/11531/2/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@472 PS2, Line 472: Name, > showPrincipal is probably a better name for this. Done http://gerrit.cloudera.org:8080/#/c/11531/2/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@474 PS2, Line 474: AnalysisException > nit: move this to L473 Done http://gerrit.cloudera.org:8080/#/c/11531/2/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@544 PS2, Line 544: ic synchronized TResultSet getUserPrivileges(String principalName, > This call can be quite expensive. We should only call it once L499 and L544 Done http://gerrit.cloudera.org:8080/#/c/11531/2/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@553 PS2, Line 553: Type.STRING.toThrift())); : addColumnOutputColumns(result.getSchema()); : resul > We can avoid this copying if we create a method, e.g. getRolePrivileges tha Done http://gerrit.cloudera.org:8080/#/c/11531/2/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/2/fe/src/test/java/org/apache/impala/service/CustomClusterGroupMapper.java@39 PS2, Line 39: > remove empty line Done http://gerrit.cloudera.org:8080/#/c/11531/2/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/2/fe/src/test/java/org/apache/impala/service/CustomClusterResourceAuthorizationProvider.java@28 PS2, Line 28: Model model) { > fix indentation Done http://gerrit.cloudera.org:8080/#/c/11531/2/fe/src/test/java/org/apache/impala/service/CustomClusterResourceAuthorizationProvider.java@33 PS2, Line 33: PolicyEngine policy, Model model) { > fix indentation Done http://gerrit.cloudera.org:8080/#/c/11531/2/testdata/workloads/functional-query/queries/QueryTest/show_grant_user.test File testdata/workloads/functional-query/queries/QueryTest/show_grant_user.test: http://gerrit.cloudera.org:8080/#/c/11531/2/testdata/workloads/functional-query/queries/QueryTest/show_grant_user.test@1 PS2, Line 1: ==== > This test cases are a bit hard to read with the names that contain numbers, The purpose of the tests is to verify different users to different groups to different role mappings. If we use one group, there we'd only be testing one user. 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@1 PS2, Line 1: # Licensed to the Apache Software Foundation (ASF) under one > nit: need consistency in this file whether to use ' or " for strings. Done http://gerrit.cloudera.org:8080/#/c/11531/2/tests/authorization/test_show_grant_user.py@18 PS2, Line 18: # Client tests for SQL statement authorization : # These tests verify the functionality of SHOW GRANT USER. We : # create several users and groups to verify clear separation. > this comment doesn't look correct. Done 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 > the finally isn't useful here. I need an finally or except to ignore the statements above that might fail because of partial test runs. Empty excepts fail flake8. http://gerrit.cloudera.org:8080/#/c/11531/2/tests/authorization/test_show_grant_user.py@87 PS2, Line 87: 'org.apache.impala.service.CustomClusterResourceAuthorizationProvi > nit: indentation Done http://gerrit.cloudera.org:8080/#/c/11531/2/tests/authorization/test_show_grant_user.py@88 PS2, Line 88: '--sentry_config={0}'.format(SENTRY_CONFIG_FILE), : catalogd_args='--sentry_config={0}'.format(SENTRY_CONFIG_FILE) + : ' --authorization_policy_provider_class=' + > use python string format instead of + Done http://gerrit.cloudera.org:8080/#/c/11531/2/tests/common/sentry_cache_test_suite.py File tests/common/sentry_cache_test_suite.py: http://gerrit.cloudera.org:8080/#/c/11531/2/tests/common/sentry_cache_test_suite.py@43 PS2, Line 43: gnore_rol > use snake_case, i.e. ignore_role Done http://gerrit.cloudera.org:8080/#/c/11531/2/tests/common/sentry_cache_test_suite.py@52 PS2, Line 52: offset > call it index? It's not the index though, it's an offset for the indexes. http://gerrit.cloudera.org:8080/#/c/11531/2/tests/common/sentry_cache_test_suite.py@52 PS2, Line 52: offset = 2 if ignore_role else 0 : # results should have the following columns for offset 0 : # scope, dat > can be simplified to index = 2 if ignore_role else 0 Done http://gerrit.cloudera.org:8080/#/c/11531/2/tests/common/sentry_cache_test_suite.py@56 PS2, Line 56: principal name, principal type, scope, database, table, column, uri, priv > Update the comment 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: 3 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 17:20:31 +0000 Gerrit-HasComments: Yes
