You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Austin Nobis (Code Review)" <ge...@cloudera.org> on 2019/05/08 20:05:23 UTC

[Impala-ASF-CR] [IMPALA-8281] Sentry frontend decoupling 

Austin Nobis has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13284


Change subject: [IMPALA-8281] Sentry frontend decoupling
......................................................................

[IMPALA-8281] Sentry frontend decoupling

This patch moves Sentry specific code from the Frontend into the
SentryImpaladAuthorizationManager.

Testing:
- Ran all FE tests
- Ran E2E authorization tests

Change-Id: Id24a00dd395e30e4c392f085893e9561da2ee539
---
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowRolesStmt.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationPolicy.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
8 files changed, 50 insertions(+), 65 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/84/13284/2
-- 
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: newchange
Gerrit-Change-Id: Id24a00dd395e30e4c392f085893e9561da2ee539
Gerrit-Change-Number: 13284
Gerrit-PatchSet: 2
Gerrit-Owner: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>

[Impala-ASF-CR] IMPALA-8281: Misc Sentry decoupling clean up

Posted by "Austin Nobis (Code Review)" <ge...@cloudera.org>.
Austin Nobis has posted comments on this change. ( http://gerrit.cloudera.org:8080/13284 )

Change subject: IMPALA-8281: Misc Sentry decoupling clean up
......................................................................


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13284/7/tests/authorization/test_sentry.py
File tests/authorization/test_sentry.py:

http://gerrit.cloudera.org:8080/#/c/13284/7/tests/authorization/test_sentry.py@93
PS7, Line 93:     finally:
            :       self.client.execute("drop database {0}".format(unique_name))
            :       self.client.execute(
> I did a quick test from master, I don't think this behavior in this CR is c
Done



-- 
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: 9
Gerrit-Owner: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 10 May 2019 16:59:38 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] [IMPALA-8281] Sentry frontend decoupling

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13284 )

Change subject: [IMPALA-8281] Sentry frontend decoupling
......................................................................


Patch Set 3:

Build Failed 

https://jenkins.impala.io/job/gerrit-code-review-checks/3131/ : Initial code review checks failed. See linked job for details on the failure.


-- 
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 <an...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 May 2019 21:19:19 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8281: Misc Sentry decoupling clean up

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13284 )

Change subject: IMPALA-8281: Misc Sentry decoupling clean up
......................................................................


Patch Set 11:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/3174/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
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: 11
Gerrit-Owner: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 10 May 2019 19:35:00 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8281: Misc Sentry decoupling clean up

Posted by "Austin Nobis (Code Review)" <ge...@cloudera.org>.
Hello Fredy Wijaya, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/13284

to look at the new patch set (#9).

Change subject: IMPALA-8281: Misc Sentry decoupling clean up
......................................................................

IMPALA-8281: Misc Sentry decoupling clean up

This patch moves Sentry specific code to the Sentry specific plugin
implementation.

Testing:
- Ran all FE tests
- Ran E2E authorization tests
- Added new E2E tests in test_sentry

Change-Id: Id24a00dd395e30e4c392f085893e9561da2ee539
---
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/analysis/CreateDropRoleStmt.java
M fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java
M fe/src/main/java/org/apache/impala/analysis/GrantRevokeRoleStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowRolesStmt.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
M tests/authorization/test_sentry.py
11 files changed, 128 insertions(+), 106 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/84/13284/9
-- 
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: newpatchset
Gerrit-Change-Id: Id24a00dd395e30e4c392f085893e9561da2ee539
Gerrit-Change-Number: 13284
Gerrit-PatchSet: 9
Gerrit-Owner: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>

[Impala-ASF-CR] IMPALA-8281: Misc Sentry decoupling clean up

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/13284 )

Change subject: IMPALA-8281: Misc Sentry decoupling clean up
......................................................................


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13284/7/tests/authorization/test_sentry.py
File tests/authorization/test_sentry.py:

http://gerrit.cloudera.org:8080/#/c/13284/7/tests/authorization/test_sentry.py@93
PS7, Line 93: 
            :       ex = self.execute_query_expect_failure(self.client, "drop role non_role")
            :       assert "" in str(ex)
this is an odd behavior, we should document this in as comment for backward compatibility and also update the commit message



-- 
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: 7
Gerrit-Owner: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 May 2019 22:31:40 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8281: Misc Sentry decoupling clean up

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13284 )

Change subject: IMPALA-8281: Misc Sentry decoupling clean up
......................................................................


Patch Set 12: Verified+1


-- 
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: 12
Gerrit-Owner: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Sat, 11 May 2019 00:09:36 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8281: Misc Sentry decoupling clean up

Posted by "Austin Nobis (Code Review)" <ge...@cloudera.org>.
Hello Fredy Wijaya, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/13284

to look at the new patch set (#7).

Change subject: IMPALA-8281: Misc Sentry decoupling clean up
......................................................................

IMPALA-8281: Misc Sentry decoupling clean up

This patch moves Sentry specific code to the Sentry specific plugin
implementation.

Testing:
- Ran all FE tests
- Ran E2E authorization tests
- Added new E2E tests in test_sentry

Change-Id: Id24a00dd395e30e4c392f085893e9561da2ee539
---
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java
M fe/src/main/java/org/apache/impala/analysis/GrantRevokeRoleStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowRolesStmt.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
M tests/authorization/test_sentry.py
10 files changed, 122 insertions(+), 92 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/84/13284/7
-- 
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: newpatchset
Gerrit-Change-Id: Id24a00dd395e30e4c392f085893e9561da2ee539
Gerrit-Change-Number: 13284
Gerrit-PatchSet: 7
Gerrit-Owner: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>

[Impala-ASF-CR] [IMPALA-8281] Sentry frontend decoupling

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
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 <an...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 May 2019 21:42:02 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8281: Misc Sentry decoupling clean up

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13284 )

Change subject: IMPALA-8281: Misc Sentry decoupling clean up
......................................................................


Patch Set 12:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4227/ DRY_RUN=false


-- 
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: 12
Gerrit-Owner: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 10 May 2019 18:41:25 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8281: Misc Sentry decoupling clean up

Posted by "Austin Nobis (Code Review)" <ge...@cloudera.org>.
Hello Fredy Wijaya, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/13284

to look at the new patch set (#6).

Change subject: IMPALA-8281: Misc Sentry decoupling clean up
......................................................................

IMPALA-8281: Misc Sentry decoupling clean up

This patch moves Sentry specific code to the Sentry specific plugin
implementation.

Testing:
- Ran all FE tests
- Ran E2E authorization tests
- Added new E2E tests in test_sentry

Change-Id: Id24a00dd395e30e4c392f085893e9561da2ee539
---
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java
M fe/src/main/java/org/apache/impala/analysis/GrantRevokeRoleStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowRolesStmt.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
M tests/authorization/test_sentry.py
10 files changed, 122 insertions(+), 92 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/84/13284/6
-- 
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: newpatchset
Gerrit-Change-Id: Id24a00dd395e30e4c392f085893e9561da2ee539
Gerrit-Change-Number: 13284
Gerrit-PatchSet: 6
Gerrit-Owner: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>

[Impala-ASF-CR] IMPALA-8281: Misc Sentry decoupling clean up

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13284 )

Change subject: IMPALA-8281: Misc Sentry decoupling clean up
......................................................................


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13284/6/tests/authorization/test_sentry.py
File tests/authorization/test_sentry.py:

http://gerrit.cloudera.org:8080/#/c/13284/6/tests/authorization/test_sentry.py@98
PS6, Line 98: 
flake8: W292 no newline at end of file



-- 
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: 6
Gerrit-Owner: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 May 2019 21:57:08 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8281: Misc Sentry decoupling clean up

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13284 )

Change subject: IMPALA-8281: Misc Sentry decoupling clean up
......................................................................


Patch Set 6:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/3162/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
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: 6
Gerrit-Owner: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 May 2019 23:10:22 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8281: Misc Sentry decoupling clean up

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13284 )

Change subject: IMPALA-8281: Misc Sentry decoupling clean up
......................................................................


Patch Set 5:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/3161/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
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: 5
Gerrit-Owner: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 May 2019 23:09:53 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] [IMPALA-8281] Sentry frontend decoupling

Posted by "Austin Nobis (Code Review)" <ge...@cloudera.org>.
Hello Fredy Wijaya, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/13284

to look at the new patch set (#3).

Change subject: [IMPALA-8281] Sentry frontend decoupling
......................................................................

[IMPALA-8281] Sentry frontend decoupling

This patch moves Sentry specific code from the Frontend into the
SentryImpaladAuthorizationManager.

Testing:
- Ran all FE tests
- Ran E2E authorization tests

Change-Id: Id24a00dd395e30e4c392f085893e9561da2ee539
---
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowRolesStmt.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
7 files changed, 49 insertions(+), 65 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/84/13284/3
-- 
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: newpatchset
Gerrit-Change-Id: Id24a00dd395e30e4c392f085893e9561da2ee539
Gerrit-Change-Number: 13284
Gerrit-PatchSet: 3
Gerrit-Owner: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>

[Impala-ASF-CR] IMPALA-8281: Misc Sentry decoupling clean up

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13284 )

Change subject: IMPALA-8281: Misc Sentry decoupling clean up
......................................................................


Patch Set 12: Code-Review+2


-- 
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: 12
Gerrit-Owner: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 10 May 2019 18:41:24 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8281: Misc Sentry decoupling clean up

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13284 )

Change subject: IMPALA-8281: Misc Sentry decoupling clean up
......................................................................


Patch Set 7:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/3163/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
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: 7
Gerrit-Owner: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 May 2019 23:38:56 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8281: Misc Sentry decoupling clean up

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/13284 )

Change subject: IMPALA-8281: Misc Sentry decoupling clean up
......................................................................


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13284/9/fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java
File fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java:

http://gerrit.cloudera.org:8080/#/c/13284/9/fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java@118
PS9, Line 118:     Role role = sentryProxy_.dropRole(requestingUser, params.getRole_name());
             :     if (role == null) {
             :       throw new AuthorizationException(String.format("Role '%s' does not exist.",
             :           params.getRole_name()));
             :     }
This code isn't the same. In this code we always ask Sentry to drop whereas previously we checked if the role was in the catalog first.



-- 
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: 9
Gerrit-Owner: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 10 May 2019 17:01:31 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8281 Sentry frontend decoupling

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/13284 )

Change subject: IMPALA-8281 Sentry frontend decoupling
......................................................................


Patch Set 5:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/13284/5//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/13284/5//COMMIT_MSG@7
PS5, Line 7: IMPALA-8281
nit add ":" after the JIRA number and maybe just say "Misc Sentry decoupling clean up"


http://gerrit.cloudera.org:8080/#/c/13284/5//COMMIT_MSG@9
PS5, Line 9: This patch moves Sentry specific code from the Frontend into the
           : SentryImpaladAuthorizationManager.
this description is only partially correct. I think we should reword it to "This patch moves Sentry specific code to Sentry specific plugin implementation".


http://gerrit.cloudera.org:8080/#/c/13284/5//COMMIT_MSG@12
PS5, Line 12: Testing:
you added a new test, right? can you update the commit message?


http://gerrit.cloudera.org:8080/#/c/13284/5/fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java
File fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java:

http://gerrit.cloudera.org:8080/#/c/13284/5/fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java@142
PS5, Line 142: existl
typo: exist


http://gerrit.cloudera.org:8080/#/c/13284/5/fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java@162
PS5, Line 162: existl
typo: exist


http://gerrit.cloudera.org:8080/#/c/13284/5/tests/authorization/test_grant_revoke.py
File tests/authorization/test_grant_revoke.py:

http://gerrit.cloudera.org:8080/#/c/13284/5/tests/authorization/test_grant_revoke.py@384
PS5, Line 384:   @pytest.mark.execute_serially
             :   @SentryCacheTestSuite.with_args(
             :     impalad_args="--server_name=server1",
             :     catalogd_args="--sentry_config=" + SENTRY_CONFIG_FILE,
             :     sentry_config=SENTRY_CONFIG_FILE)
             :   def test_grant_revoke_invalid_role(self, unique_name):
             :     role_name = "foobar"
             :     try:
             :       # This will create "foobar" role catalog object.
             :       self.client.execute("create role {0}".format(role_name))
             :       self.client.execute("grant all on server to {0}".format(role_name))
             :       self.client.execute("grant role {0} to group `{1}`".format(
             :         role_name, grp.getgrnam(getuser()).gr_name))
             :       self.client.execute("create database {0}".format(unique_name))
             : 
             :       ex = self.execute_query_expect_failure(
             :         self.client, "grant all on database {0} to role non_role".format(unique_name))
             :       assert "Role 'non_role' does not exist" in str(ex)
             : 
             :       ex = self.execute_query_expect_failure(
             :         self.client, "revoke all on database {0} from role non_role".format(unique_name))
             :       assert "Role 'non_role' does not exist" in str(ex)
             : 
             :       ex = self.execute_query_expect_failure(self.client, "show grant role non_role")
             :       assert "Role 'non_role' does not exist" in str(ex)
             :     finally:
             :       self.client.execute("drop database {0}".format(unique_name))
             :       self.client.execute("drop role {0}".format(role_name))
we should move this test to test_sentry.py instead. It doesn't catch the typo error I pointed out so i think we need to have more coverage on this :)
- drop role non_role
- show grant role non_role
- grant role non_role to group



-- 
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: 5
Gerrit-Owner: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 09 May 2019 21:26:15 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8281: Misc Sentry decoupling clean up

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/13284 )

Change subject: IMPALA-8281: Misc Sentry decoupling clean up
......................................................................


Patch Set 11: Code-Review+2


-- 
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: 11
Gerrit-Owner: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 10 May 2019 18:41:05 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] [IMPALA-8281] Sentry frontend decoupling

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13284 )

Change subject: [IMPALA-8281] Sentry frontend decoupling
......................................................................


Patch Set 2:

Build Failed 

https://jenkins.impala.io/job/gerrit-code-review-checks/3130/ : Initial code review checks failed. See linked job for details on the failure.


-- 
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: 2
Gerrit-Owner: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 May 2019 21:19:27 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8281: Misc Sentry decoupling clean up

Posted by "Austin Nobis (Code Review)" <ge...@cloudera.org>.
Hello Fredy Wijaya, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/13284

to look at the new patch set (#8).

Change subject: IMPALA-8281: Misc Sentry decoupling clean up
......................................................................

IMPALA-8281: Misc Sentry decoupling clean up

This patch moves Sentry specific code to the Sentry specific plugin
implementation.

Testing:
- Ran all FE tests
- Ran E2E authorization tests
- Added new E2E tests in test_sentry

Change-Id: Id24a00dd395e30e4c392f085893e9561da2ee539
---
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java
M fe/src/main/java/org/apache/impala/analysis/GrantRevokeRoleStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowRolesStmt.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
M tests/authorization/test_sentry.py
10 files changed, 119 insertions(+), 92 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/84/13284/8
-- 
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: newpatchset
Gerrit-Change-Id: Id24a00dd395e30e4c392f085893e9561da2ee539
Gerrit-Change-Number: 13284
Gerrit-PatchSet: 8
Gerrit-Owner: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>

[Impala-ASF-CR] IMPALA-8281: Misc Sentry decoupling clean up

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13284 )

Change subject: IMPALA-8281: Misc Sentry decoupling clean up
......................................................................


Patch Set 9:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/3170/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
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: 9
Gerrit-Owner: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 10 May 2019 17:56:48 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8281: Misc Sentry decoupling clean up

Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/13284 )

Change subject: IMPALA-8281: Misc Sentry decoupling clean up
......................................................................


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13284/7/tests/authorization/test_sentry.py
File tests/authorization/test_sentry.py:

http://gerrit.cloudera.org:8080/#/c/13284/7/tests/authorization/test_sentry.py@93
PS7, Line 93:     finally:
            :       self.client.execute("drop database {0}".format(unique_name))
            :       self.client.execute(
> My mistake. This test case is invalid. Calling `drop role` on a non-existen
I did a quick test from master, I don't think this behavior in this CR is correct.

[localhost:21000] default> drop role non_role;
Query: drop role non_role
ERROR: AnalysisException: Role 'non_role' does not exist.



-- 
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: 8
Gerrit-Owner: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 10 May 2019 16:36:35 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8281: Misc Sentry decoupling clean up

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13284 )

Change subject: IMPALA-8281: Misc Sentry decoupling clean up
......................................................................


Patch Set 10:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/3171/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
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: 10
Gerrit-Owner: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 10 May 2019 18:07:30 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8281: Misc Sentry decoupling clean up

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13284 )

Change subject: IMPALA-8281: Misc Sentry decoupling clean up
......................................................................

IMPALA-8281: Misc Sentry decoupling clean up

This patch moves Sentry specific code to the Sentry specific plugin
implementation.

Testing:
- Ran all FE tests
- Ran E2E authorization tests
- Added new E2E tests in test_sentry

Change-Id: Id24a00dd395e30e4c392f085893e9561da2ee539
Reviewed-on: http://gerrit.cloudera.org:8080/13284
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/analysis/CreateDropRoleStmt.java
M fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java
M fe/src/main/java/org/apache/impala/analysis/GrantRevokeRoleStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowRolesStmt.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
M tests/authorization/test_sentry.py
11 files changed, 140 insertions(+), 104 deletions(-)

Approvals:
  Impala Public Jenkins: Looks good to me, approved; Verified

-- 
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: merged
Gerrit-Change-Id: Id24a00dd395e30e4c392f085893e9561da2ee539
Gerrit-Change-Number: 13284
Gerrit-PatchSet: 13
Gerrit-Owner: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>

[Impala-ASF-CR] IMPALA-8281: Misc Sentry decoupling clean up

Posted by "Austin Nobis (Code Review)" <ge...@cloudera.org>.
Austin Nobis has uploaded a new patch set (#11). ( http://gerrit.cloudera.org:8080/13284 )

Change subject: IMPALA-8281: Misc Sentry decoupling clean up
......................................................................

IMPALA-8281: Misc Sentry decoupling clean up

This patch moves Sentry specific code to the Sentry specific plugin
implementation.

Testing:
- Ran all FE tests
- Ran E2E authorization tests
- Added new E2E tests in test_sentry

Change-Id: Id24a00dd395e30e4c392f085893e9561da2ee539
---
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/analysis/CreateDropRoleStmt.java
M fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java
M fe/src/main/java/org/apache/impala/analysis/GrantRevokeRoleStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowRolesStmt.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
M tests/authorization/test_sentry.py
11 files changed, 140 insertions(+), 104 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/84/13284/11
-- 
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: newpatchset
Gerrit-Change-Id: Id24a00dd395e30e4c392f085893e9561da2ee539
Gerrit-Change-Number: 13284
Gerrit-PatchSet: 11
Gerrit-Owner: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>

[Impala-ASF-CR] IMPALA-8281: Misc Sentry decoupling clean up

Posted by "Austin Nobis (Code Review)" <ge...@cloudera.org>.
Austin Nobis has uploaded a new patch set (#10). ( http://gerrit.cloudera.org:8080/13284 )

Change subject: IMPALA-8281: Misc Sentry decoupling clean up
......................................................................

IMPALA-8281: Misc Sentry decoupling clean up

This patch moves Sentry specific code to the Sentry specific plugin
implementation.

Testing:
- Ran all FE tests
- Ran E2E authorization tests
- Added new E2E tests in test_sentry

Change-Id: Id24a00dd395e30e4c392f085893e9561da2ee539
---
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/analysis/CreateDropRoleStmt.java
M fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java
M fe/src/main/java/org/apache/impala/analysis/GrantRevokeRoleStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowRolesStmt.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
M tests/authorization/test_sentry.py
11 files changed, 140 insertions(+), 104 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/84/13284/10
-- 
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: newpatchset
Gerrit-Change-Id: Id24a00dd395e30e4c392f085893e9561da2ee539
Gerrit-Change-Number: 13284
Gerrit-PatchSet: 10
Gerrit-Owner: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>

[Impala-ASF-CR] IMPALA-8281: Misc Sentry decoupling clean up

Posted by "Austin Nobis (Code Review)" <ge...@cloudera.org>.
Austin Nobis has posted comments on this change. ( http://gerrit.cloudera.org:8080/13284 )

Change subject: IMPALA-8281: Misc Sentry decoupling clean up
......................................................................


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13284/7/tests/authorization/test_sentry.py
File tests/authorization/test_sentry.py:

http://gerrit.cloudera.org:8080/#/c/13284/7/tests/authorization/test_sentry.py@93
PS7, Line 93:     finally:
            :       self.client.execute("drop database {0}".format(unique_name))
            :       self.client.execute(
> this is an odd behavior, we should document this in as comment for backward
My mistake. This test case is invalid. Calling `drop role` on a non-existent role does not result in an exception.



-- 
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: 8
Gerrit-Owner: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 10 May 2019 15:39:54 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8281 Sentry frontend decoupling

Posted by "Austin Nobis (Code Review)" <ge...@cloudera.org>.
Austin Nobis has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/13284 )

Change subject: IMPALA-8281 Sentry frontend decoupling
......................................................................

IMPALA-8281 Sentry frontend decoupling

This patch moves Sentry specific code from the Frontend into the
SentryImpaladAuthorizationManager.

Testing:
- Ran all FE tests
- Ran E2E authorization tests

Change-Id: Id24a00dd395e30e4c392f085893e9561da2ee539
---
M common/thrift/Frontend.thrift
M fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java
M fe/src/main/java/org/apache/impala/analysis/GrantRevokeRoleStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java
M fe/src/main/java/org/apache/impala/analysis/ShowRolesStmt.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryImpaladAuthorizationManager.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
M tests/authorization/test_grant_revoke.py
10 files changed, 113 insertions(+), 92 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/84/13284/5
-- 
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: newpatchset
Gerrit-Change-Id: Id24a00dd395e30e4c392f085893e9561da2ee539
Gerrit-Change-Number: 13284
Gerrit-PatchSet: 5
Gerrit-Owner: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>

[Impala-ASF-CR] IMPALA-8281: Misc Sentry decoupling clean up

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13284 )

Change subject: IMPALA-8281: Misc Sentry decoupling clean up
......................................................................


Patch Set 8:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/3169/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
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: 8
Gerrit-Owner: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 10 May 2019 16:25:09 +0000
Gerrit-HasComments: No