----------------------------------------------------------- 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 > >
