You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by Na Li via Review Board <no...@reviews.apache.org> on 2018/09/20 04:24:18 UTC
Review Request 68779: SENTRY-2409: ALTER TABLE SET OWNER does not
allow to change the table if using only the table name
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68779/
-----------------------------------------------------------
Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio Pena.
Bugs: sentry-2409
https://issues.apache.org/jira/browse/sentry-2409
Repository: sentry
Description
-------
add code to get table name and DB name based on token children size
Diffs
-----
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java 6731d1a
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java 55a79ee
Diff: https://reviews.apache.org/r/68779/diff/1/
Testing
-------
unit tests in TestOwnerPrivileges passed
Thanks,
Na Li
Re: Review Request 68779: SENTRY-2409: ALTER TABLE SET OWNER does not
allow to change the table if using only the table name
Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.
> On Sept. 20, 2018, 2:20 p.m., Sergio Pena wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
> > Lines 725 (patched)
> > <https://reviews.apache.org/r/68779/diff/1/?file=2090407#file2090407line725>
> >
> > This line fails immediatly, but instead of throwing a test case error, it goes to the finally() clause and finish with the test successfully.
>
> Na Li wrote:
> why do you think this fails? It should work.
It failed to me while running the debug. The test does not fail, but the line jumps directly to the finally() clause.
- Sergio
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68779/#review208796
-----------------------------------------------------------
On Sept. 20, 2018, 4:24 a.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68779/
> -----------------------------------------------------------
>
> (Updated Sept. 20, 2018, 4:24 a.m.)
>
>
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio Pena.
>
>
> Bugs: sentry-2409
> https://issues.apache.org/jira/browse/sentry-2409
>
>
> Repository: sentry
>
>
> Description
> -------
>
> add code to get table name and DB name based on token children size
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java 6731d1a
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java 55a79ee
>
>
> Diff: https://reviews.apache.org/r/68779/diff/1/
>
>
> Testing
> -------
>
> unit tests in TestOwnerPrivileges passed
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 68779: SENTRY-2409: ALTER TABLE SET OWNER does not
allow to change the table if using only the table name
Posted by Na Li via Review Board <no...@reviews.apache.org>.
> On Sept. 20, 2018, 2:20 p.m., Sergio Pena wrote:
> > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
> > Lines 288-300 (patched)
> > <https://reviews.apache.org/r/68779/diff/1/?file=2090406#file2090406line288>
> >
> > Is it possible to reuse the extractDatabase() and extractTable() commands that already does most of this split of db and table names?
extractDatabase() and extractTable() are deprecated, and we should not use them any more. I found we can use extractDbTableNameFromTOKTABLE(), which sets output DB and table. So I need to change alterTableSetOwnerPrivilege in HiveAuthzPrivilegesMap to use output db and table instead of input.
In this way, it is more consistent with alterDbSetOwnerPrivilege, which also uses output db.
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68779/#review208796
-----------------------------------------------------------
On Sept. 20, 2018, 4:24 a.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68779/
> -----------------------------------------------------------
>
> (Updated Sept. 20, 2018, 4:24 a.m.)
>
>
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio Pena.
>
>
> Bugs: sentry-2409
> https://issues.apache.org/jira/browse/sentry-2409
>
>
> Repository: sentry
>
>
> Description
> -------
>
> add code to get table name and DB name based on token children size
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java 6731d1a
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java 55a79ee
>
>
> Diff: https://reviews.apache.org/r/68779/diff/1/
>
>
> Testing
> -------
>
> unit tests in TestOwnerPrivileges passed
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 68779: SENTRY-2409: ALTER TABLE SET OWNER does not
allow to change the table if using only the table name
Posted by Na Li via Review Board <no...@reviews.apache.org>.
> On Sept. 20, 2018, 2:20 p.m., Sergio Pena wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
> > Lines 680 (patched)
> > <https://reviews.apache.org/r/68779/diff/1/?file=2090407#file2090407line680>
> >
> > Do you need to insert data for this test case? I don't think is necessary. Inserts are expensive in Hive, so removing it will save time on the test execution.
I will remove it
> On Sept. 20, 2018, 2:20 p.m., Sergio Pena wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
> > Lines 682 (patched)
> > <https://reviews.apache.org/r/68779/diff/1/?file=2090407#file2090407line682>
> >
> > Code convention: separate if and ( by a space.
> >
> > Btw, I noticed that this test case has two different test paths depending on the value of this flag. I don't see this a good practice when writing test cases because if you change the flag for an unknown reason, then you will never execute the other test case and you won't know if it fails.
> >
> > I'd rather write another test case for the grant option enabled instead of putting logic conditions in test cases.
I will split the test into two test cases: one for positive and one for negative
> On Sept. 20, 2018, 2:20 p.m., Sergio Pena wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
> > Lines 710-717 (patched)
> > <https://reviews.apache.org/r/68779/diff/1/?file=2090407#file2090407line710>
> >
> > This test case is a negative test case and it can be put in different method.
will split it out
> On Sept. 20, 2018, 2:20 p.m., Sergio Pena wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
> > Lines 716-717 (patched)
> > <https://reviews.apache.org/r/68779/diff/1/?file=2090407#file2090407line716>
> >
> > How do we validate that the error was not authorized and it was not a syntax error?
will use HiveSqlException
> On Sept. 20, 2018, 2:20 p.m., Sergio Pena wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
> > Lines 725 (patched)
> > <https://reviews.apache.org/r/68779/diff/1/?file=2090407#file2090407line725>
> >
> > This line fails immediatly, but instead of throwing a test case error, it goes to the finally() clause and finish with the test successfully.
why do you think this fails? It should work.
> On Sept. 20, 2018, 2:20 p.m., Sergio Pena wrote:
> > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
> > Lines 733 (patched)
> > <https://reviews.apache.org/r/68779/diff/1/?file=2090407#file2090407line733>
> >
> > Aren't we running this test case in other parts of this class? Seems redundant to test that the ALTER TABLE fails with a table that does not exist.
> >
> > I prefer to see different methods for each test case you're trying to validate instead of putting several into one method.
I will remove it
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68779/#review208796
-----------------------------------------------------------
On Sept. 20, 2018, 4:24 a.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68779/
> -----------------------------------------------------------
>
> (Updated Sept. 20, 2018, 4:24 a.m.)
>
>
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio Pena.
>
>
> Bugs: sentry-2409
> https://issues.apache.org/jira/browse/sentry-2409
>
>
> Repository: sentry
>
>
> Description
> -------
>
> add code to get table name and DB name based on token children size
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java 6731d1a
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java 55a79ee
>
>
> Diff: https://reviews.apache.org/r/68779/diff/1/
>
>
> Testing
> -------
>
> unit tests in TestOwnerPrivileges passed
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 68779: SENTRY-2409: ALTER TABLE SET OWNER does not
allow to change the table if using only the table name
Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68779/#review208796
-----------------------------------------------------------
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java
Lines 288-300 (patched)
<https://reviews.apache.org/r/68779/#comment292998>
Is it possible to reuse the extractDatabase() and extractTable() commands that already does most of this split of db and table names?
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
Lines 680 (patched)
<https://reviews.apache.org/r/68779/#comment292992>
Do you need to insert data for this test case? I don't think is necessary. Inserts are expensive in Hive, so removing it will save time on the test execution.
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
Lines 682 (patched)
<https://reviews.apache.org/r/68779/#comment292993>
Code convention: separate if and ( by a space.
Btw, I noticed that this test case has two different test paths depending on the value of this flag. I don't see this a good practice when writing test cases because if you change the flag for an unknown reason, then you will never execute the other test case and you won't know if it fails.
I'd rather write another test case for the grant option enabled instead of putting logic conditions in test cases.
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
Lines 710-717 (patched)
<https://reviews.apache.org/r/68779/#comment292994>
This test case is a negative test case and it can be put in different method.
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
Lines 716-717 (patched)
<https://reviews.apache.org/r/68779/#comment292995>
How do we validate that the error was not authorized and it was not a syntax error?
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
Lines 725 (patched)
<https://reviews.apache.org/r/68779/#comment292996>
This line fails immediatly, but instead of throwing a test case error, it goes to the finally() clause and finish with the test successfully.
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java
Lines 733 (patched)
<https://reviews.apache.org/r/68779/#comment292997>
Aren't we running this test case in other parts of this class? Seems redundant to test that the ALTER TABLE fails with a table that does not exist.
I prefer to see different methods for each test case you're trying to validate instead of putting several into one method.
- Sergio Pena
On Sept. 20, 2018, 4:24 a.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68779/
> -----------------------------------------------------------
>
> (Updated Sept. 20, 2018, 4:24 a.m.)
>
>
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio Pena.
>
>
> Bugs: sentry-2409
> https://issues.apache.org/jira/browse/sentry-2409
>
>
> Repository: sentry
>
>
> Description
> -------
>
> add code to get table name and DB name based on token children size
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java 6731d1a
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java 55a79ee
>
>
> Diff: https://reviews.apache.org/r/68779/diff/1/
>
>
> Testing
> -------
>
> unit tests in TestOwnerPrivileges passed
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 68779: SENTRY-2409: ALTER TABLE SET OWNER does not
allow to change the table if using only the table name
Posted by Sergio Pena via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68779/#review209149
-----------------------------------------------------------
Ship it!
Ship It!
- Sergio Pena
On Oct. 1, 2018, 9:23 p.m., Na Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68779/
> -----------------------------------------------------------
>
> (Updated Oct. 1, 2018, 9:23 p.m.)
>
>
> Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio Pena.
>
>
> Bugs: sentry-2409
> https://issues.apache.org/jira/browse/sentry-2409
>
>
> Repository: sentry
>
>
> Description
> -------
>
> add code to get table name and DB name based on token children size
>
>
> Diffs
> -----
>
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java 6731d1a
> sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java 385ca98
> sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java 55a79ee
>
>
> Diff: https://reviews.apache.org/r/68779/diff/2/
>
>
> Testing
> -------
>
> unit tests in TestOwnerPrivileges passed
>
>
> Thanks,
>
> Na Li
>
>
Re: Review Request 68779: SENTRY-2409: ALTER TABLE SET OWNER does not
allow to change the table if using only the table name
Posted by Na Li via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68779/
-----------------------------------------------------------
(Updated Oct. 1, 2018, 9:23 p.m.)
Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio Pena.
Bugs: sentry-2409
https://issues.apache.org/jira/browse/sentry-2409
Repository: sentry
Description
-------
add code to get table name and DB name based on token children size
Diffs (updated)
-----
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java 6731d1a
sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/HiveAuthzPrivilegesMap.java 385ca98
sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java 55a79ee
Diff: https://reviews.apache.org/r/68779/diff/2/
Changes: https://reviews.apache.org/r/68779/diff/1-2/
Testing
-------
unit tests in TestOwnerPrivileges passed
Thanks,
Na Li