> On July 11, 2016, 9:17 a.m., Hao Hao wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/Fencer.java, > > line 161 > > <https://reviews.apache.org/r/49738/diff/4/?file=1439872#file1439872line161> > > > > Exception will be thrown if create table failed?
yes > On July 11, 2016, 9:17 a.m., Hao Hao wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/Fencer.java, > > line 222 > > <https://reviews.apache.org/r/49738/diff/4/?file=1439872#file1439872line222> > > > > Should it be expected? It seems it should never happen? Good question. It is not a harmful condition, so there is no reason to consider it an error. It could happen if a daemon loses the leader election but then regains the leadership shortly thereafter. > On July 11, 2016, 9:17 a.m., Hao Hao wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/Fencer.java, > > line 234 > > <https://reviews.apache.org/r/49738/diff/4/?file=1439872#file1439872line234> > > > > Yeah, not clear about what is the function for? It seems not be called > > anywhere? see above comment-- the purpose is to verify that the fencing table still exists as part of a transaction. There will be callers later > On July 11, 2016, 9:17 a.m., Hao Hao wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 175 > > <https://reviews.apache.org/r/49738/diff/4/?file=1439874#file1439874line175> > > > > Could you document a bit more why do we need these two properties to > > disallow operations outside of transcations? Will it ever happen? This is an existing setting of SentryStore-- nothing is being changed here, just moving around some code. See the section about "non-transactional reads" here: http://www.datanucleus.org/products/datanucleus/jdo/performance_tuning.html to see why we don't want them > On July 11, 2016, 9:17 a.m., Hao Hao wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java, > > line 156 > > <https://reviews.apache.org/r/49738/diff/4/?file=1439877#file1439877line156> > > > > Why do we need to removeth haContext? HAContext was used by the old HA implementation which is being replaced. There is no function for it any more. > On July 11, 2016, 9:17 a.m., Hao Hao wrote: > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreIntegrationBase.java, > > line 36 > > <https://reviews.apache.org/r/49738/diff/4/?file=1439884#file1439884line36> > > > > Should we add a e2e test for fencing between active and standby? I definitely agree that more tests would be good. We have that on the roadmap, and shouldn't block this patch on that, though. - Colin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49738/#review141650 ----------------------------------------------------------- On July 8, 2016, 7:59 p.m., Colin McCabe wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49738/ > ----------------------------------------------------------- > > (Updated July 8, 2016, 7:59 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 > >
