Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11039 )
Change subject: IMPALA-7342: Add initial support for user-level permissions ...................................................................... Patch Set 18: (6 comments) http://gerrit.cloudera.org:8080/#/c/11039/18/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java File fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java: http://gerrit.cloudera.org:8080/#/c/11039/18/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@49 PS18, Line 49: role/user perhaps define this as a PrincipalType, then use it in the remaining places here? http://gerrit.cloudera.org:8080/#/c/11039/18/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@59 PS18, Line 59: he principal ID to using the princi couldn't parse this (nor could I before). perhaps replace 'using' with 'probe'? http://gerrit.cloudera.org:8080/#/c/11039/18/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@68 PS18, Line 68: name nit: names http://gerrit.cloudera.org:8080/#/c/11039/18/fe/src/main/java/org/apache/impala/catalog/AuthorizationPolicy.java@239 PS18, Line 239: Principal I can see that it makes sense to model the Thrift as an object that contains a type. Would it make more sense to model it in Java using separate classes so that we get basic type-checking? For example, the return type here is more specifically List<Role>. A Principal as super-class can still make sense when you have heterogeneous collections. Thoughts? http://gerrit.cloudera.org:8080/#/c/11039/18/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/11039/18/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@449 PS18, Line 449: for (Principal user: getAllUsers()) { : addPrincipalToCatalogDelta(user, ctx); : } why is this change here? I was expecting a change like this in the patch that addresses users. http://gerrit.cloudera.org:8080/#/c/11039/18/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java File fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java: http://gerrit.cloudera.org:8080/#/c/11039/18/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@2326 PS18, Line 2326: super(test); pull these up to single line where applicable. -- To view, visit http://gerrit.cloudera.org:8080/11039 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I07e0d46d2e50d35bd64ee573b5aa4b779eb9e62f Gerrit-Change-Number: 11039 Gerrit-PatchSet: 18 Gerrit-Owner: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Adam Holley <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Tue, 14 Aug 2018 06:40:37 +0000 Gerrit-HasComments: Yes
