[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests

[Tim Armstrong (Code Review)]

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 
Reviewed-by: Vuk Ercegovac 
---
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 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Vuk Ercegovac 

[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests

[Tim Armstrong (Code Review)]

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 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Mon, 18 Jun 2018 22:38:29 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests

[Vuk Ercegovac (Code Review)]

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 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Mon, 18 Jun 2018 05:12:03 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests

[Impala Public Jenkins (Code Review)]

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 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Fri, 15 Jun 2018 18:43:54 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests

[Impala Public Jenkins (Code Review)]

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 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Fri, 15 Jun 2018 15:20:26 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests

[Adam Holley (Code Review)]

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 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Vuk Ercegovac 

[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests

[Vuk Ercegovac (Code Review)]

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 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Thu, 14 Jun 2018 23:14:15 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests

[Tim Armstrong (Code Review)]

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 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Thu, 14 Jun 2018 22:29:06 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests

[Vuk Ercegovac (Code Review)]

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 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Wed, 13 Jun 2018 23:53:52 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests

[Adam Holley (Code Review)]

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 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Tue, 12 Jun 2018 21:48:43 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests

[Adam Holley (Code Review)]

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 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Vuk Ercegovac 

[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests

[Adam Holley (Code Review)]

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 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Tue, 12 Jun 2018 21:48:18 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests

[Vuk Ercegovac (Code Review)]

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 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Fri, 08 Jun 2018 22:02:14 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests

[Fredy Wijaya (Code Review)]

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 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Fri, 08 Jun 2018 16:51:43 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests

[Adam Holley (Code Review)]

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 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Vuk Ercegovac 

[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests

[Fredy Wijaya (Code Review)]

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 
--
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 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Vuk Ercegovac 

[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests

[Fredy Wijaya (Code Review)]

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 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Comment-Date: Wed, 23 May 2018 14:30:14 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests

[Fredy Wijaya (Code Review)]

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  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  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 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Comment-Date: Wed, 23 May 2018 14:29:55 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests

[Adam Holley (Code Review)]

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  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 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Comment-Date: Wed, 23 May 2018 07:29:46 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests

[Adam Holley (Code Review)]

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 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Fredy Wijaya 

[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests

[Fredy Wijaya (Code Review)]

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  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  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 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Comment-Date: Tue, 22 May 2018 23:11:45 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6802 (part 4): Clean up authorization tests

[Adam Holley (Code Review)]

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 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Fredy Wijaya