Philip Zeyliger has posted comments on this change. ( )

Change subject: IMPALA-6817: Clean up Impala privilege model

Patch Set 7:


Looks fine to me. I have a few recommendations for test additions, but nothing 

Have you run the code coverage tool (it's pretty pleasant; you get HTML output; 
look for "jacoco") to make sure the new code is covered by the new tests?
PS7, Line 43:     for (ImpalaAction action : ImpalaAction.values()) {
Is this something like

action = ImpalaAction.valueOf(ImpalaAction.class, name.toLower())
return new BitFieldAction(action.getValue(), action.getCode())
// obviously check that it's lower vs upper, and that this handles
// the "not found" case well.

I don't think the current implementation is problematic, but I think enum has a 
built-in for this.
File fe/src/main/java/org/apache/impala/authorization/
PS7, Line 71:     ImpalaAction(String value, int code) {
I thought about suggesting "1 << (this.ordinal() + 1)", but on balance I think 
explicit is better.
PS7, Line 29: public class ImpalaActionFactoryTest {
Please add a test that all the codes except ALL are unique and that ALL is the 
"bitwise OR" of all of them. I know you've done the latter implicitly, but I 
think explicitly would be good too. Basically, I'm worried about someone 
accidentally adding something into the list in the future and forgetting to 
update something.
PS7, Line 80:         impala.getActionByName("select"));
Please add a test with mixed and upper case.
PS7, Line 92:         impala.getActionByName("*"));
should getActionByName("all") work too?

To view, visit
To unsubscribe, visit

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I65f3f9b4d06f3b03bfa2f484737bb746ec598a6b
Gerrit-Change-Number: 9942
Gerrit-PatchSet: 7
Gerrit-Owner: Fredy Wijaya <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Fredy Wijaya <>
Gerrit-Reviewer: Philip Zeyliger <>
Gerrit-Comment-Date: Fri, 06 Apr 2018 16:46:12 +0000
Gerrit-HasComments: Yes

Reply via email to