> On July 15, 2016, 12:17 a.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java, > > line 76 > > <https://reviews.apache.org/r/49996/diff/1/?file=1443345#file1443345line76> > > > > Not related to this patch. But, seems like get(String) can be made > > private in Activators?
Good idea. Done. > On July 15, 2016, 12:17 a.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/Fencer.java, > > line 250 > > <https://reviews.apache.org/r/49996/diff/1/?file=1443347#file1443347line250> > > > > Annotate with @VisibleForTesting as seems like this is test only thing? k > On July 15, 2016, 12:17 a.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/Activator.java, > > line 122 > > <https://reviews.apache.org/r/49996/diff/1/?file=1443349#file1443349line122> > > > > When should Activator.isActive() versus Activator.verifyActive() be > > used? > > > > Seems like verifyActive() is the ultimate ground truth, is there a risk > > in usage of isActive around failover? isActive is used for a different purpose. It returns if the daemon thinks that it is active. This is the state needed by metrics and other places where the daemon is managing its internal state according to whether it is active or not. Of course, having isActive doesn't negate the need for fencing, since fencing ensures that the SQL database also thinks that we're active. I renamed Activator#verifyActive to Activator#checkSqlFencing to make the distinction clearer. I also added some JavaDoc to isActive. > On July 15, 2016, 12:17 a.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestFencer.java, > > line 52 > > <https://reviews.apache.org/r/49996/diff/1/?file=1443350#file1443350line52> > > > > Nit: > > Does it make sense to do > > this.act = Activators.INSTANCE.createActivators(conf) > > > > rather than the following in multiple places: > > > > this.act = new Activator() > > Activators.INSTANCE.put(act) > > > > Also, shall we restrict the use of Activators purely for testing > > purposes? Sure, let's create a function called Activators#create which can both create the Activator and add it to Activators. Activators is not restricted to testing. It's a singleton which is basically a hack around how difficult (and incompatible) it is to change the constructors of all the SentryStore and other related classes to have more parameters than a single Configuration object. With the incarnation ID stored in the Configuration, code can retrieve the correct Activator. - Colin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49996/#review142306 ----------------------------------------------------------- On July 13, 2016, 6:28 p.m., Colin McCabe wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49996/ > ----------------------------------------------------------- > > (Updated July 13, 2016, 6:28 p.m.) > > > Review request for sentry, Hao Hao and Sravya Tirukkovalur. > > > Bugs: SENTRY-1399 > https://issues.apache.org/jira/browse/SENTRY-1399 > > > Repository: sentry > > > Description > ------- > > SENTRY-1399: Integrate Fencer with SentryStore > > > Diffs > ----- > > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/exception/SentryStandbyException.java > 73c7e4e > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java > e960dcd > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java > c003965 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/Fencer.java > 14cdde3 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 6e367e5 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/Activator.java > 0b7ddf5 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestFencer.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java > 6e00505 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStoreImportExport.java > fc39658 > > Diff: https://reviews.apache.org/r/49996/diff/ > > > Testing > ------- > > > Thanks, > > Colin McCabe > >
