----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49738/#review141285 -----------------------------------------------------------
Nice work Colin! Left some initial feedback. Seems like we need to add testing to make sure fencing is being done? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/Fencer.java (line 105) <https://reviews.apache.org/r/49738/#comment206771> Document which exceptions are thrown in what conditions? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/Fencer.java (line 120) <https://reviews.apache.org/r/49738/#comment206770> SentryAccessDeniedException does not seem like the right exception we want to throw here? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/Fencer.java (line 126) <https://reviews.apache.org/r/49738/#comment206772> Same as above, is this the right exception type? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/Fencer.java (line 135) <https://reviews.apache.org/r/49738/#comment206773> Why rollback? What does roll back mean for read only queries? Is it still recommended to commit the transaction? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/Fencer.java (line 171) <https://reviews.apache.org/r/49738/#comment206774> Excecute within a transaction? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/Fencer.java (line 174) <https://reviews.apache.org/r/49738/#comment206775> Rollback if it has'nt been successful? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/Fencer.java (line 206) <https://reviews.apache.org/r/49738/#comment206776> The purpose of this function does not look accurate? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SqlAccessor.java (line 70) <https://reviews.apache.org/r/49738/#comment206768> We do not have support for sql server in Sentry. So best to leave it out for now? sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestPrivilegeOperatePersistence.java (line 992) <https://reviews.apache.org/r/49738/#comment206778> It is unclear to me what new validations are we adding to this test? - Sravya Tirukkovalur On July 7, 2016, 9:33 p.m., Colin McCabe wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49738/ > ----------------------------------------------------------- > > (Updated July 7, 2016, 9:33 p.m.) > > > Review request for sentry, Hao Hao, Rahul Sharma, and Sravya Tirukkovalur. > > > Bugs: SENTRY-1317 > https://issues.apache.org/jira/browse/SENTRY-1317 > > > Repository: sentry > > > Description > ------- > > SENTRY-1317: Implement fencing required for active/passive > > > Diffs > ----- > > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryStandbyException.java > PRE-CREATION > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java > 3da4906 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/PluginCacheSyncUtil.java > 4ce16c7 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryHdfsMetricsUtil.java > 5bf2f6e > > sentry-hdfs/sentry-hdfs-service/src/test/java/org/apache/sentry/hdfs/TestHAUpdateForwarder.java > 5246e05 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/Fencer.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java > cacc29f > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 7dad496 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/ServiceRegister.java > 79dfe48 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SqlAccessor.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java > 3de1f65 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/Activator.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/Activators.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatus.java > e846766 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusAdaptor.java > 80a6571 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > 809af06 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > 0ab8192 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreIntegrationBase.java > f14b586 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestPrivilegeOperatePersistence.java > 799d5ef > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > 3ef1cf7 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreImportExport.java > 3ff97df > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryVersion.java > a8e8a03 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/SentryServiceIntegrationBase.java > cb2d9c9 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestLeaderStatus.java > 434ac41 > > Diff: https://reviews.apache.org/r/49738/diff/ > > > Testing > ------- > > > Thanks, > > Colin McCabe > >
