Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201707131702.v6DH2wSh029213@ip-10-146-233-104.ec2.internal>
Date: Thu, 13 Jul 2017 17:02:58 +0000
From: "Bharath Vissapragada (Code Review)" <gerrit@cloudera.org>
To: anujphadke <aphadke@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
CC: Matthew Jacobs <mj@cloudera.com>
Reply-To: bharathv@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-5582=3A_Store_sentry_privileges_in_lower_case=0A?=
In-Reply-To: <gerrit.1498723251000.Ide3dfa601fcf77f5acc6adce9bea443aea600901@gerrit.cloudera.org>
References: <gerrit.1498723251000.Ide3dfa601fcf77f5acc6adce9bea443aea600901@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.7
archived-at: Thu, 13 Jul 2017 17:03:12 -0000

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