----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22120/#review44505 -----------------------------------------------------------
Mostly looks good, minor comments below. sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java <https://reviews.apache.org/r/22120/#comment78901> This can also return void as the other constructor, as context object is visible from the child class( it is protected in parent). sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java <https://reviews.apache.org/r/22120/#comment78902> context object is available from parent class. sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java <https://reviews.apache.org/r/22120/#comment78903> This is not needed, as there is a cleaner teardown function in the parent class which also stops the server. sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java <https://reviews.apache.org/r/22120/#comment78904> context is already created in @Before sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java <https://reviews.apache.org/r/22120/#comment78906> load_data role never used? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java <https://reviews.apache.org/r/22120/#comment78909> Comment seems incorrect, we are trying to create a table here rather than dropping a db. sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java <https://reviews.apache.org/r/22120/#comment78905> Not needed (See above) sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java <https://reviews.apache.org/r/22120/#comment78907> load_data role never used? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java <https://reviews.apache.org/r/22120/#comment78910> Same as above, comment seems incorrect. - Sravya Tirukkovalur On May 31, 2014, 10:13 a.m., Prasad Mujumdar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22120/ > ----------------------------------------------------------- > > (Updated May 31, 2014, 10:13 a.m.) > > > Review request for sentry and Sravya Tirukkovalur. > > > Bugs: SENTRY-241 > https://issues.apache.org/jira/browse/SENTRY-241 > > > Repository: sentry > > > Description > ------- > > The Hive binding grantRevokeTask should fire the Sentry on-failure hook if > configured. > > > Diffs > ----- > > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/hadoop/hive/ql/exec/SentryGrantRevokeTask.java > f1e6247 > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java > f8c694c > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/AbstractTestWithDbProvider.java > c6f1ce2 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbSentryOnFailureHookLoading.java > PRE-CREATION > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/DummySentryOnFailureHook.java > e4055a7 > > Diff: https://reviews.apache.org/r/22120/diff/ > > > Testing > ------- > > Added new test cases. > > > Thanks, > > Prasad Mujumdar > >
