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 16: (11 comments) initial comments. will get to the bigger ones next (AuthorizationPolicy, CatalogServiceCatalog, SentryProxy, tests) http://gerrit.cloudera.org:8080/#/c/11039/16/common/thrift/CatalogObjects.thrift File common/thrift/CatalogObjects.thrift: http://gerrit.cloudera.org:8080/#/c/11039/16/common/thrift/CatalogObjects.thrift@480 PS16, Line 480: // Represents a principal type add a comment about what is a principal since its unclear what its used for. might make sense for the comment to explain TPrincipal below. if this maps to some concept in Sentry, then perhaps it makes sense to add a link to docs there. http://gerrit.cloudera.org:8080/#/c/11039/16/fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java File fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java: http://gerrit.cloudera.org:8080/#/c/11039/16/fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java@46 PS16, Line 46: role rename to principal_ the intent is to extend this stmt to users as well? if so, perhaps add a todo to change roleName_ http://gerrit.cloudera.org:8080/#/c/11039/16/fe/src/main/java/org/apache/impala/analysis/ShowGrantRoleStmt.java File fe/src/main/java/org/apache/impala/analysis/ShowGrantRoleStmt.java: http://gerrit.cloudera.org:8080/#/c/11039/16/fe/src/main/java/org/apache/impala/analysis/ShowGrantRoleStmt.java@32 PS16, Line 32: ROLE this will always apply only to role, not user, if I understand it correctly (otherwise, I'd expect a "show grant user ..."). would it make sense to assert that role_ is always of the ROLE flavor? http://gerrit.cloudera.org:8080/#/c/11039/16/fe/src/main/java/org/apache/impala/catalog/Catalog.java File fe/src/main/java/org/apache/impala/catalog/Catalog.java: http://gerrit.cloudera.org:8080/#/c/11039/16/fe/src/main/java/org/apache/impala/catalog/Catalog.java@517 PS16, Line 517: + use String.format http://gerrit.cloudera.org:8080/#/c/11039/16/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java File fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java: http://gerrit.cloudera.org:8080/#/c/11039/16/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java@473 PS16, Line 473: thriftPrincipal.getPrincipal_name(), thriftPrincipal.getPrincipal_type() why not simplify and make the arg here just thriftPrincipal? http://gerrit.cloudera.org:8080/#/c/11039/16/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java@477 PS16, Line 477: thriftPrincipal.getPrincipal_name(), : thriftPrincipal.getPrincipal_type() same question here. http://gerrit.cloudera.org:8080/#/c/11039/16/fe/src/main/java/org/apache/impala/catalog/Principal.java File fe/src/main/java/org/apache/impala/catalog/Principal.java: http://gerrit.cloudera.org:8080/#/c/11039/16/fe/src/main/java/org/apache/impala/catalog/Principal.java@66 PS16, Line 66: will be nit: is http://gerrit.cloudera.org:8080/#/c/11039/16/fe/src/main/java/org/apache/impala/catalog/Principal.java@84 PS16, Line 84: privilegeName is there any assumption about case with the name? http://gerrit.cloudera.org:8080/#/c/11039/16/fe/src/main/java/org/apache/impala/catalog/Principal.java@128 PS16, Line 128: this nit: to this http://gerrit.cloudera.org:8080/#/c/11039/16/fe/src/main/java/org/apache/impala/catalog/Principal.java@129 PS16, Line 129: the principal nit: remove http://gerrit.cloudera.org:8080/#/c/11039/16/fe/src/main/java/org/apache/impala/catalog/Principal.java@167 PS16, Line 167: Role why does the case differ from the all-caps on L150? -- 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: 16 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 02:06:46 +0000 Gerrit-HasComments: Yes
