Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/13284 )
Change subject: [IMPALA-8281] Sentry frontend decoupling ...................................................................... Patch Set 3: (8 comments) http://gerrit.cloudera.org:8080/#/c/13284/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13284/3//COMMIT_MSG@7 PS3, Line 7: [IMPALA-8281] nit: IMPALA-8281: http://gerrit.cloudera.org:8080/#/c/13284/3/common/thrift/Frontend.thrift File common/thrift/Frontend.thrift: http://gerrit.cloudera.org:8080/#/c/13284/3/common/thrift/Frontend.thrift@267 PS3, Line 267: // 2: required bool is_admin_op add: REMOVED - prefix http://gerrit.cloudera.org:8080/#/c/13284/3/common/thrift/Frontend.thrift@295 PS3, Line 295: // 4: required bool is_admin_op add: REMOVED - prefix http://gerrit.cloudera.org:8080/#/c/13284/3/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/13284/3/fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java@36 PS3, Line 36: loaded nit: authorization provider? http://gerrit.cloudera.org:8080/#/c/13284/3/fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java@39 PS3, Line 39: public class GrantRevokePrivStmt extends AuthorizationStmt { can we refactor this logic here: https://github.com/apache/impala/blob/4c6ac151ef2c6efac8a8a3d02c342334bf9d688c/fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java#L105-L107 http://gerrit.cloudera.org:8080/#/c/13284/3/fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java@44 PS3, Line 44: private final TPrincipalType principalType_; can we also refactor the GrantRevokeRoleStmt? https://github.com/apache/impala/blob/4c6ac151ef2c6efac8a8a3d02c342334bf9d688c/fe/src/main/java/org/apache/impala/analysis/GrantRevokeRoleStmt.java#L60-L62 http://gerrit.cloudera.org:8080/#/c/13284/3/fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java File fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java: http://gerrit.cloudera.org:8080/#/c/13284/3/fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java@106 PS3, Line 106: SHOW ROLE typo: SHOW ROLES? http://gerrit.cloudera.org:8080/#/c/13284/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java: http://gerrit.cloudera.org:8080/#/c/13284/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java@118 PS3, Line 118: } We don't want to reduce the test coverage, if it's not possible to test it in FE test, can we write an E2E test instead? Make sure we have tests for whatever code that we refactor. -- To view, visit http://gerrit.cloudera.org:8080/13284 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id24a00dd395e30e4c392f085893e9561da2ee539 Gerrit-Change-Number: 13284 Gerrit-PatchSet: 3 Gerrit-Owner: Austin Nobis <ano...@cloudera.com> Gerrit-Reviewer: Austin Nobis <ano...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Wed, 08 May 2019 21:42:02 +0000 Gerrit-HasComments: Yes
