impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dimitris Tsirogiannis (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-5355: Fix the order of Sentry roles and privileges
Date Tue, 30 May 2017 18:40:55 GMT
Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-5355: Fix the order of Sentry roles and privileges

Patch Set 1:

File fe/src/main/java/org/apache/impala/catalog/

PS1, Line 1203: Preconditions.checkState(role.checkVersionConsistency());
I don't think this check is needed here. I don't see how, given that the catalogLock is held,
you could end up in an inconsistent state.

PS1, Line 1224: Preconditions.checkState(role.checkVersionConsistency());
same here
File fe/src/main/java/org/apache/impala/catalog/

Line 122:     Preconditions.checkState(result.checkVersionConsistency());
add an error message here, something to indicate the problem.
File tests/authorization/

PS1, Line 30: TestImpaladRestart
Add description of what kind of tests we plan on adding in this class.

PS1, Line 50: test_impalad_restart
This function name is a bit generic and it's not clear what this test case is all about.

PS1, Line 51: correclty
typo: correctly

PS1, Line 57: assert
It's also important for this test that there are privileges associated with this role. You
may want to add a show grant privileges request to verify that this is indeed the case. The
same check should be added after the restart.

PS1, Line 64: sleep(10)
I believe there is a better way to detect this. I think there is a metric (CATALOG_READY or
something like this) you can query that indicates if the impalad has received the catalog
update and is ready to process requests.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I7072e95b74952ce5a51ea1b6e2ae3e80fb0940e0
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Dimitris Tsirogiannis <>
Gerrit-HasComments: Yes

View raw message