Bharath Vissapragada has posted comments on this change. Change subject: IMPALA-5582: Store sentry privileges in lower case ......................................................................
Patch Set 3: (9 comments) Few more in addition to MJ's comments. http://gerrit.cloudera.org:8080/#/c/7332/3//COMMIT_MSG Commit Message: PS3, Line 16: If the catalogObjectCache on a update adds the new : rolePrivilege object first and removes the old object later qq, Isn't this order always the same? http://gerrit.cloudera.org:8080/#/c/7332/3/be/src/catalog/catalog.cc File be/src/catalog/catalog.cc: PS3, Line 41: DEFINE_int64 int32 should be fine. PS3, Line 41: sentry_catalog_polling_frequency how about sentry_catalog_polling_frequency_s ? (indicates seconds) http://gerrit.cloudera.org:8080/#/c/7332/3/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java File fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java: PS3, Line 336: Fix GRANTs on URIs with uppercase letters s/ URIs are case sensitive ? http://gerrit.cloudera.org:8080/#/c/7332/3/fe/src/main/java/org/apache/impala/catalog/RolePrivilege.java File fe/src/main/java/org/apache/impala/catalog/RolePrivilege.java: Line 75: toLowerCase())); nit: 4 space formatting everywhere http://gerrit.cloudera.org:8080/#/c/7332/3/tests/authorization/test_grant_revoke.py File tests/authorization/test_grant_revoke.py: PS3, Line 130: statestored_args=("--statestore_heartbeat_frequency_ms=300 " : "--statestore_update_frequency_ms=300")) Is this required? PS3, Line 143: grant_rev_db Looks like a common db name used in authz tests. Could you randomize it? Else we might hit flaky tests if this DB already exists from test_grant_revoke for ex. Line 149: self.client.execute("grant select on table test1 to test_role") Could you also add some grants on camel case DB names? PS3, Line 165: self.client.execute("drop role test_role") How about adding it to a finally block incase the test fails, also clean up the dbs created? -- To view, visit http://gerrit.cloudera.org:8080/7332 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ide3dfa601fcf77f5acc6adce9bea443aea600901 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: anujphadke <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: anujphadke <[email protected]> Gerrit-HasComments: Yes
