You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Adam Holley (Code Review)" <ge...@cloudera.org> on 2018/05/17 18:21:05 UTC
[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests
Adam Holley has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10442
Change subject: IMPALA-6802 (part 4): Clean up authorization tests
......................................................................
IMPALA-6802 (part 4): Clean up authorization tests
The third part of this patch is to rewrite the following authorization
tests:
- describe
Testing:
- Added new authorization tests
- Ran all front-end tests
Change-Id: Ic4cf3a4751b41908ef81ec35d89a2713d9fa0dc4
Cherry-picks: not for 2.x
---
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
1 file changed, 262 insertions(+), 35 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/42/10442/1
--
To view, visit http://gerrit.cloudera.org:8080/10442
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic4cf3a4751b41908ef81ec35d89a2713d9fa0dc4
Gerrit-Change-Number: 10442
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10442 )
Change subject: IMPALA-6802 (part 4): Clean up authorization tests
......................................................................
Patch Set 3: Code-Review+1
Vuk, can you take a look at it?
--
To view, visit http://gerrit.cloudera.org:8080/10442
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic4cf3a4751b41908ef81ec35d89a2713d9fa0dc4
Gerrit-Change-Number: 10442
Gerrit-PatchSet: 3
Gerrit-Owner: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Jun 2018 16:51:43 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests
Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.
Adam Holley has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/10442 )
Change subject: IMPALA-6802 (part 4): Clean up authorization tests
......................................................................
IMPALA-6802 (part 4): Clean up authorization tests
The fourth part of this patch is to rewrite the following authorization
tests:
- describe
Testing:
- Added new authorization tests
- Ran all front-end tests
Cherry-picks: not for 2.x
Change-Id: Ic4cf3a4751b41908ef81ec35d89a2713d9fa0dc4
---
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
1 file changed, 251 insertions(+), 36 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/42/10442/3
--
To view, visit http://gerrit.cloudera.org:8080/10442
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic4cf3a4751b41908ef81ec35d89a2713d9fa0dc4
Gerrit-Change-Number: 10442
Gerrit-PatchSet: 3
Gerrit-Owner: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10442 )
Change subject: IMPALA-6802 (part 4): Clean up authorization tests
......................................................................
Patch Set 1:
(11 comments)
Taking this over since Adam is out.
http://gerrit.cloudera.org:8080/#/c/10442/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java:
http://gerrit.cloudera.org:8080/#/c/10442/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@104
PS1, Line 104: private static final String[] ALLTYPES_COLUMNS = (String []) ArrayUtils.addAll(
> nit: String[]
Done
http://gerrit.cloudera.org:8080/#/c/10442/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@923
PS1, Line 923: .ok(onServer(TPrivilegeLevel.INSERT))
> We need to add REFRESH since VIEW_METADATA consists of SELECT, INSERT, and
Done
http://gerrit.cloudera.org:8080/#/c/10442/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@927
PS1, Line 927: .error(accessError("functional"));
> Missing check .error(accessError("functional"), onDatabase...)
Done
http://gerrit.cloudera.org:8080/#/c/10442/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@933
PS1, Line 933: TTableName tableName = new TTableName("functional","alltypes");
> nit: space after comma
Done
http://gerrit.cloudera.org:8080/#/c/10442/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@935
PS1, Line 935: authorize("describe functional.alltypes")
> describe <table> uses ANY privilege, we need more test cases with all other
Done
http://gerrit.cloudera.org:8080/#/c/10442/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@961
PS1, Line 961: String [] locationString = new String[]{"Location:"};
> nit: no space for array declaration
Done
http://gerrit.cloudera.org:8080/#/c/10442/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@989
PS1, Line 989: tableName = new TTableName("functional","alltypes_view");
> nit: space after comma
Done
http://gerrit.cloudera.org:8080/#/c/10442/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@1013
PS1, Line 1013: tableName = new TTableName("functional","alltypes_view");
> nit: space after comma
Done
http://gerrit.cloudera.org:8080/#/c/10442/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@1018
PS1, Line 1018: viewStrings);
> join L1018 with L1017
Done
http://gerrit.cloudera.org:8080/#/c/10442/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@1040
PS1, Line 1040: authorize("describe functional.allcomplextypes.int_struct_col")
> describe <table> uses ANY privilege, we need more test cases with all other
Done
http://gerrit.cloudera.org:8080/#/c/10442/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@1183
PS1, Line 1183: String [] requiredStrings, String [] excludedStrings,
> nit: fix indentation
Done
--
To view, visit http://gerrit.cloudera.org:8080/10442
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic4cf3a4751b41908ef81ec35d89a2713d9fa0dc4
Gerrit-Change-Number: 10442
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Comment-Date: Wed, 23 May 2018 14:29:55 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10442 )
Change subject: IMPALA-6802 (part 4): Clean up authorization tests
......................................................................
Patch Set 2: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/10442
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic4cf3a4751b41908ef81ec35d89a2713d9fa0dc4
Gerrit-Change-Number: 10442
Gerrit-PatchSet: 2
Gerrit-Owner: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Comment-Date: Wed, 23 May 2018 14:30:14 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has removed a vote on this change.
Change subject: IMPALA-6802 (part 4): Clean up authorization tests
......................................................................
Removed Code-Review+1 by Fredy Wijaya <fw...@cloudera.com>
--
To view, visit http://gerrit.cloudera.org:8080/10442
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Ic4cf3a4751b41908ef81ec35d89a2713d9fa0dc4
Gerrit-Change-Number: 10442
Gerrit-PatchSet: 2
Gerrit-Owner: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10442 )
Change subject: IMPALA-6802 (part 4): Clean up authorization tests
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2684/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/10442
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic4cf3a4751b41908ef81ec35d89a2713d9fa0dc4
Gerrit-Change-Number: 10442
Gerrit-PatchSet: 5
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 15 Jun 2018 15:20:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10442 )
Change subject: IMPALA-6802 (part 4): Clean up authorization tests
......................................................................
IMPALA-6802 (part 4): Clean up authorization tests
The fourth part of this patch is to rewrite the following authorization
tests:
- describe
Testing:
- Added new authorization tests
- Ran all front-end tests
Cherry-picks: not for 2.x
Change-Id: Ic4cf3a4751b41908ef81ec35d89a2713d9fa0dc4
Reviewed-on: http://gerrit.cloudera.org:8080/10442
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Reviewed-by: Vuk Ercegovac <ve...@cloudera.com>
---
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
1 file changed, 279 insertions(+), 57 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Vuk Ercegovac: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/10442
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic4cf3a4751b41908ef81ec35d89a2713d9fa0dc4
Gerrit-Change-Number: 10442
Gerrit-PatchSet: 6
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10442 )
Change subject: IMPALA-6802 (part 4): Clean up authorization tests
......................................................................
Patch Set 1:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/10442/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java:
http://gerrit.cloudera.org:8080/#/c/10442/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@104
PS1, Line 104: private static final String[] ALLTYPES_COLUMNS = (String []) ArrayUtils.addAll(
nit: String[]
http://gerrit.cloudera.org:8080/#/c/10442/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@923
PS1, Line 923: .ok(onServer(TPrivilegeLevel.INSERT))
We need to add REFRESH since VIEW_METADATA consists of SELECT, INSERT, and REFRESH
http://gerrit.cloudera.org:8080/#/c/10442/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@927
PS1, Line 927: .error(accessError("functional"));
Missing check .error(accessError("functional"), onDatabase...)
http://gerrit.cloudera.org:8080/#/c/10442/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@933
PS1, Line 933: TTableName tableName = new TTableName("functional","alltypes");
nit: space after comma
http://gerrit.cloudera.org:8080/#/c/10442/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@935
PS1, Line 935: authorize("describe functional.alltypes")
describe <table> uses ANY privilege, we need more test cases with all other privileges.
http://gerrit.cloudera.org:8080/#/c/10442/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@961
PS1, Line 961: String [] locationString = new String[]{"Location:"};
nit: no space for array declaration
http://gerrit.cloudera.org:8080/#/c/10442/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@989
PS1, Line 989: tableName = new TTableName("functional","alltypes_view");
nit: space after comma
http://gerrit.cloudera.org:8080/#/c/10442/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@1013
PS1, Line 1013: tableName = new TTableName("functional","alltypes_view");
nit: space after comma
http://gerrit.cloudera.org:8080/#/c/10442/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@1018
PS1, Line 1018: viewStrings);
join L1018 with L1017
http://gerrit.cloudera.org:8080/#/c/10442/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@1040
PS1, Line 1040: authorize("describe functional.allcomplextypes.int_struct_col")
describe <table> uses ANY privilege, we need more test cases with all other privileges.
http://gerrit.cloudera.org:8080/#/c/10442/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@1183
PS1, Line 1183: String [] requiredStrings, String [] excludedStrings,
nit: fix indentation
--
To view, visit http://gerrit.cloudera.org:8080/10442
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic4cf3a4751b41908ef81ec35d89a2713d9fa0dc4
Gerrit-Change-Number: 10442
Gerrit-PatchSet: 1
Gerrit-Owner: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Comment-Date: Tue, 22 May 2018 23:11:45 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests
Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.
Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10442 )
Change subject: IMPALA-6802 (part 4): Clean up authorization tests
......................................................................
Patch Set 3: Code-Review+1
(6 comments)
updated. carry +1
http://gerrit.cloudera.org:8080/#/c/10442/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java:
http://gerrit.cloudera.org:8080/#/c/10442/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@918
PS3, Line 918: @Test
> perhaps add a comment explaining what's being tested... from a first glance
Done
http://gerrit.cloudera.org:8080/#/c/10442/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@922
PS3, Line 922:
> consistent spacing (see comment below)
Done
http://gerrit.cloudera.org:8080/#/c/10442/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@938
PS3, Line 938:
> and several more places below
Done
http://gerrit.cloudera.org:8080/#/c/10442/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@946
PS3, Line 946: allExcept(
: TPrivilegeLevel.ALL, TPrivilegeLevel.SELECT)
> factor out (repeated 3 times in this block, so it adds too much noise).
This is the pattern we've been using in these tests to be explicit for each test on the privileges required. It adds text, but reduces the need to reference what the variable or constant means in a different part of the code.
http://gerrit.cloudera.org:8080/#/c/10442/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@952
PS3, Line 952: ng[]{"id"},
> explain what is being tested here (missing why "id" is special)
Done
http://gerrit.cloudera.org:8080/#/c/10442/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@1181
PS3, Line 1181:
> use consistent spacing for this... from a brief look, seems like no space i
Done
--
To view, visit http://gerrit.cloudera.org:8080/10442
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic4cf3a4751b41908ef81ec35d89a2713d9fa0dc4
Gerrit-Change-Number: 10442
Gerrit-PatchSet: 3
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Jun 2018 21:48:18 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10442 )
Change subject: IMPALA-6802 (part 4): Clean up authorization tests
......................................................................
Patch Set 4:
I think this should be ok to merge whenever Vuk is happy with it.
--
To view, visit http://gerrit.cloudera.org:8080/10442
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic4cf3a4751b41908ef81ec35d89a2713d9fa0dc4
Gerrit-Change-Number: 10442
Gerrit-PatchSet: 4
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 14 Jun 2018 22:29:06 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests
Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.
Adam Holley has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/10442 )
Change subject: IMPALA-6802 (part 4): Clean up authorization tests
......................................................................
IMPALA-6802 (part 4): Clean up authorization tests
The fourth part of this patch is to rewrite the following authorization
tests:
- describe
Testing:
- Added new authorization tests
- Ran all front-end tests
Cherry-picks: not for 2.x
Change-Id: Ic4cf3a4751b41908ef81ec35d89a2713d9fa0dc4
---
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
1 file changed, 279 insertions(+), 57 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/42/10442/5
--
To view, visit http://gerrit.cloudera.org:8080/10442
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic4cf3a4751b41908ef81ec35d89a2713d9fa0dc4
Gerrit-Change-Number: 10442
Gerrit-PatchSet: 5
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests
Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.
Adam Holley has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/10442 )
Change subject: IMPALA-6802 (part 4): Clean up authorization tests
......................................................................
IMPALA-6802 (part 4): Clean up authorization tests
The fourth part of this patch is to rewrite the following authorization
tests:
- describe
Testing:
- Added new authorization tests
- Ran all front-end tests
Cherry-picks: not for 2.x
Change-Id: Ic4cf3a4751b41908ef81ec35d89a2713d9fa0dc4
---
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
1 file changed, 277 insertions(+), 57 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/42/10442/4
--
To view, visit http://gerrit.cloudera.org:8080/10442
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic4cf3a4751b41908ef81ec35d89a2713d9fa0dc4
Gerrit-Change-Number: 10442
Gerrit-PatchSet: 4
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/10442 )
Change subject: IMPALA-6802 (part 4): Clean up authorization tests
......................................................................
Patch Set 5:
Seems ok to go ahead and merge. There haven't been any related commits that have gone in since you ran the tests.
--
To view, visit http://gerrit.cloudera.org:8080/10442
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic4cf3a4751b41908ef81ec35d89a2713d9fa0dc4
Gerrit-Change-Number: 10442
Gerrit-PatchSet: 5
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Mon, 18 Jun 2018 22:38:29 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/10442 )
Change subject: IMPALA-6802 (part 4): Clean up authorization tests
......................................................................
Patch Set 4:
I'm fine with it.. one more small change needed to comment style
--
To view, visit http://gerrit.cloudera.org:8080/10442
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic4cf3a4751b41908ef81ec35d89a2713d9fa0dc4
Gerrit-Change-Number: 10442
Gerrit-PatchSet: 4
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Thu, 14 Jun 2018 23:14:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10442 )
Change subject: IMPALA-6802 (part 4): Clean up authorization tests
......................................................................
Patch Set 5: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/10442
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic4cf3a4751b41908ef81ec35d89a2713d9fa0dc4
Gerrit-Change-Number: 10442
Gerrit-PatchSet: 5
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 15 Jun 2018 18:43:54 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests
Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.
Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10442 )
Change subject: IMPALA-6802 (part 4): Clean up authorization tests
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/10442/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java:
http://gerrit.cloudera.org:8080/#/c/10442/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@923
PS1, Line 923: authzTest.ok(onServer(privilege))
> We need to add REFRESH since VIEW_METADATA consists of SELECT, INSERT, and
I think this should be changed so it’s only select. Different Jira to fix though.
http://gerrit.cloudera.org:8080/#/c/10442/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@935
PS1, Line 935: TTableName tableName = new TTableName("functional", "alltypes");
> describe <table> uses ANY privilege, we need more test cases with all other
Same as for database. I think only select should allow describe.
http://gerrit.cloudera.org:8080/#/c/10442/1/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@1018
PS1, Line 1018: .okDescribe(tableName, style, checkStrings, null, onTable("functional",
> join L1018 with L1017
These need to be separate since then view text does not show for insert.
--
To view, visit http://gerrit.cloudera.org:8080/10442
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic4cf3a4751b41908ef81ec35d89a2713d9fa0dc4
Gerrit-Change-Number: 10442
Gerrit-PatchSet: 2
Gerrit-Owner: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Comment-Date: Wed, 23 May 2018 07:29:46 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/10442 )
Change subject: IMPALA-6802 (part 4): Clean up authorization tests
......................................................................
Patch Set 4: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10442/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java:
http://gerrit.cloudera.org:8080/#/c/10442/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@946
PS3, Line 946: ctional",
: "alltypes", privilege));
> This is the pattern we've been using in these tests to be explicit for each
hmm.. ok, guess it takes a bit of getting used to
http://gerrit.cloudera.org:8080/#/c/10442/4/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java:
http://gerrit.cloudera.org:8080/#/c/10442/4/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@919
PS4, Line 919: //
use /** style comments for methods.
--
To view, visit http://gerrit.cloudera.org:8080/10442
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic4cf3a4751b41908ef81ec35d89a2713d9fa0dc4
Gerrit-Change-Number: 10442
Gerrit-PatchSet: 4
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Wed, 13 Jun 2018 23:53:52 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/10442 )
Change subject: IMPALA-6802 (part 4): Clean up authorization tests
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/10442
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic4cf3a4751b41908ef81ec35d89a2713d9fa0dc4
Gerrit-Change-Number: 10442
Gerrit-PatchSet: 5
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Mon, 18 Jun 2018 05:12:03 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests
Posted by "Vuk Ercegovac (Code Review)" <ge...@cloudera.org>.
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/10442 )
Change subject: IMPALA-6802 (part 4): Clean up authorization tests
......................................................................
Patch Set 3:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/10442/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java:
http://gerrit.cloudera.org:8080/#/c/10442/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@918
PS3, Line 918: @Test
perhaps add a comment explaining what's being tested... from a first glance, it looks like certain privs can see certain fields, and others cannot? if there are docs or jiras that explain this more, pls reference them here.
http://gerrit.cloudera.org:8080/#/c/10442/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@922
PS3, Line 922:
consistent spacing (see comment below)
http://gerrit.cloudera.org:8080/#/c/10442/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@938
PS3, Line 938:
and several more places below
http://gerrit.cloudera.org:8080/#/c/10442/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@946
PS3, Line 946: allExcept(
: TPrivilegeLevel.ALL, TPrivilegeLevel.SELECT)
factor out (repeated 3 times in this block, so it adds too much noise).
http://gerrit.cloudera.org:8080/#/c/10442/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@952
PS3, Line 952: ng[]{"id"},
explain what is being tested here (missing why "id" is special)
http://gerrit.cloudera.org:8080/#/c/10442/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@1181
PS3, Line 1181:
use consistent spacing for this... from a brief look, seems like no space is the preference.
--
To view, visit http://gerrit.cloudera.org:8080/10442
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic4cf3a4751b41908ef81ec35d89a2713d9fa0dc4
Gerrit-Change-Number: 10442
Gerrit-PatchSet: 3
Gerrit-Owner: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Jun 2018 22:02:14 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests
Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.
Adam Holley has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/10442 )
Change subject: IMPALA-6802 (part 4): Clean up authorization tests
......................................................................
IMPALA-6802 (part 4): Clean up authorization tests
The fourth part of this patch is to rewrite the following authorization
tests:
- describe
Testing:
- Added new authorization tests
- Ran all front-end tests
Cherry-picks: not for 2.x
Change-Id: Ic4cf3a4751b41908ef81ec35d89a2713d9fa0dc4
---
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
1 file changed, 251 insertions(+), 36 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/42/10442/2
--
To view, visit http://gerrit.cloudera.org:8080/10442
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic4cf3a4751b41908ef81ec35d89a2713d9fa0dc4
Gerrit-Change-Number: 10442
Gerrit-PatchSet: 2
Gerrit-Owner: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests
Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.
Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10442 )
Change subject: IMPALA-6802 (part 4): Clean up authorization tests
......................................................................
Patch Set 4: Code-Review+1
Carry +1
--
To view, visit http://gerrit.cloudera.org:8080/10442
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic4cf3a4751b41908ef81ec35d89a2713d9fa0dc4
Gerrit-Change-Number: 10442
Gerrit-PatchSet: 4
Gerrit-Owner: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Adam Holley <ah...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Vuk Ercegovac <ve...@cloudera.com>
Gerrit-Comment-Date: Tue, 12 Jun 2018 21:48:43 +0000
Gerrit-HasComments: No