impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bharath Vissapragada (Code Review)" <>
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:


Few more in addition to MJ's comments.
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?
File be/src/catalog/

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)
File fe/src/main/java/org/apache/impala/catalog/

PS3, Line 336: Fix GRANTs on URIs with uppercase letters
s/ URIs are case sensitive ?
File fe/src/main/java/org/apache/impala/catalog/

Line 75:                 toLowerCase()));
nit: 4 space formatting everywhere
File tests/authorization/

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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Ide3dfa601fcf77f5acc6adce9bea443aea600901
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: anujphadke <>
Gerrit-Reviewer: Bharath Vissapragada <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: anujphadke <>
Gerrit-HasComments: Yes

View raw message