Fredy Wijaya 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: (11 comments) I have to rebase because of merge conflicts. 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 that maps to Sentry principal type. > add a comment about what is a principal since its unclear what its used for Done 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: prin > rename to principal_ Yes, the intent is to extend this statement to users as well. Done. 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 There's a separate JIRA for SHOW GRANT USER: https://issues.apache.org/jira/browse/IMPALA-6989. I have CR for that JIRA that I haven't published yet pending this CR. 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 Done 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? The problem is there's an overloaded version of AuthorizationPolicy.getPrincipal(int principalId, TPrincipalType type). Interestingly, TPrincipal has two fields principal_name and principal_id. TPrivilege also has a property that contains principal_id. Creating AuthorizationPolicy.getPrincipal(TPrincipal) when we always use the principal_name can be somewhat confusing. 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. Same explanation as above. 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: is retu > nit: is Done 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? The case shouldn't matter. We just take what Sentry returns. http://gerrit.cloudera.org:8080/#/c/11039/16/fe/src/main/java/org/apache/impala/catalog/Principal.java@128 PS16, Line 128: to t > nit: to this Done http://gerrit.cloudera.org:8080/#/c/11039/16/fe/src/main/java/org/apache/impala/catalog/Principal.java@129 PS16, Line 129: . > nit: remove Done 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? This is mostly used for printing an error message. By making "Role" or "User". It's easy to make it all upper case or all lower case by calling .toUpperCase() and toLowerCase() respectively. -- 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 03:43:06 +0000 Gerrit-HasComments: Yes
