Philip Zeyliger has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9942 )

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


Patch Set 7:

(5 comments)

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

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?

http://gerrit.cloudera.org:8080/#/c/9942/7/fe/src/compat-minicluster-profile-3/java/org/apache/impala/authorization/ImpalaActionFactory.java
File 
fe/src/compat-minicluster-profile-3/java/org/apache/impala/authorization/ImpalaActionFactory.java:

http://gerrit.cloudera.org:8080/#/c/9942/7/fe/src/compat-minicluster-profile-3/java/org/apache/impala/authorization/ImpalaActionFactory.java@43
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.


http://gerrit.cloudera.org:8080/#/c/9942/7/fe/src/main/java/org/apache/impala/authorization/Privilege.java
File fe/src/main/java/org/apache/impala/authorization/Privilege.java:

http://gerrit.cloudera.org:8080/#/c/9942/7/fe/src/main/java/org/apache/impala/authorization/Privilege.java@71
PS7, Line 71:     ImpalaAction(String value, int code) {
I thought about suggesting "1 << (this.ordinal() + 1)", but on balance I think 
explicit is better.


http://gerrit.cloudera.org:8080/#/c/9942/7/fe/src/test/java/org/apache/impala/authorization/ImpalaActionFactoryTest.java
File 
fe/src/test/java/org/apache/impala/authorization/ImpalaActionFactoryTest.java:

http://gerrit.cloudera.org:8080/#/c/9942/7/fe/src/test/java/org/apache/impala/authorization/ImpalaActionFactoryTest.java@29
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.


http://gerrit.cloudera.org:8080/#/c/9942/7/fe/src/test/java/org/apache/impala/authorization/ImpalaActionFactoryTest.java@80
PS7, Line 80:         impala.getActionByName("select"));
Please add a test with mixed and upper case.


http://gerrit.cloudera.org:8080/#/c/9942/7/fe/src/test/java/org/apache/impala/authorization/ImpalaActionFactoryTest.java@92
PS7, Line 92:         impala.getActionByName("*"));
should getActionByName("all") work too?



--
To view, visit http://gerrit.cloudera.org:8080/9942
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

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 <fwij...@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com>
Gerrit-Comment-Date: Fri, 06 Apr 2018 16:46:12 +0000
Gerrit-HasComments: Yes

Reply via email to