impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bharath Vissapragada (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5582: Store sentry privileges in lower case
Date Thu, 13 Jul 2017 17:02:58 GMT
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 <aphadke@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bharathv@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: anujphadke <aphadke@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message