> On June 21, 2016, 12:55 p.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatus.java, > > lines 80-84 > > <https://reviews.apache.org/r/48761/diff/1/?file=1420478#file1420478line80> > > > > Curious if there is an advantage of using rand here versus > > monotonically icnreasing id? > > Colin McCabe wrote: > Hmm. This has to be unique across the whole cluster. Using a > monotonically increasing id would require some kind of synchronization across > the whole cluster to avoid reusing IDs.
Ah, right. We do not have a shared Id. > On June 21, 2016, 12:55 p.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatus.java, > > lines 105-111 > > <https://reviews.apache.org/r/48761/diff/1/?file=1420478#file1420478line105> > > > > Why different calls for HA and non HA case? Seems like > > listener.becomeActive is setting the isActive flag in SentryService. This > > means, that flag will never be set in HA cases? > > Colin McCabe wrote: > The non-HA case doesn't make use of Curator/ZooKeeper. Instead, it just > activates itself all the time. So there isn't any reason to create a > leaderStatusAdaptor in the non-HA case. I was curious why listener.becomeActive is not called in HA case. My bad, I see that we are handling that inside the adaptor. > On June 21, 2016, 12:55 p.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusAdaptor.java, > > line 163 > > <https://reviews.apache.org/r/48761/diff/1/?file=1420479#file1420479line163> > > > > What is the behavior if becomeStandby fails? There might be more than > > one leader? > > Colin McCabe wrote: > There is never more than one leader. If becomeStandby fails, then the > current daemon has failed to become the leader and it will release its ZK > resources. hm. I do not see a throws Exception contract for the becomeStandby() function in Listener interface? > On June 21, 2016, 12:55 p.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java, > > line 172 > > <https://reviews.apache.org/r/48761/diff/1/?file=1420480#file1420480line172> > > > > Is the comment still relevant? > > Colin McCabe wrote: > This is still relevant. Probably the start() method should be moved into > call, but that's follow-on work. > > The rationale for moving this method into call() is that when we perform > callbacks, it is nice to know that the object we're calling into is fully > constructed, which we can't be sure of in the constructor. Sounds good. > On June 21, 2016, 12:55 p.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatus.java, > > line 75 > > <https://reviews.apache.org/r/48761/diff/1/?file=1420478#file1420478line75> > > > > May be comment on why is this an AtomicBoolean? Need for thread safety? > > Colin McCabe wrote: > I used AtomicBoolean so that closing LeaderStatus more than once only > causes close() to run once. I'll add a comment. Sounds good. But, I am trying to understand if there a need for LeaderStatus to be thread safe? If not, there is no chance of close being called concurrently right? Serial calls to close() is not a problem with a plain boolean. - Sravya ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48761/#review138824 ----------------------------------------------------------- On June 22, 2016, 6:31 p.m., Colin McCabe wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48761/ > ----------------------------------------------------------- > > (Updated June 22, 2016, 6:31 p.m.) > > > Review request for sentry. > > > Bugs: SENTRY-1316 > https://issues.apache.org/jira/browse/SENTRY-1316 > > > Repository: sentry > > > Description > ------- > > Implement Sentry leadership election > > > Diffs > ----- > > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/SentryHDFSServiceClientFactory.java > 6c9c8bb > > sentry-hdfs/sentry-hdfs-common/src/main/java/org/apache/sentry/hdfs/ha/HdfsHAClientInvocationHandler.java > 6138b8c > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/MetastorePluginWithHA.java > 6476a01 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarder.java > 7387281 > > sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/UpdateForwarderWithHA.java > 574627c > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/ServiceManager.java > 9f921d4 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/HAClientInvocationHandler.java > d97a07e > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatus.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusAdaptor.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > 6883bf4 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientFactory.java > 48ee66a > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryServiceClientPoolFactory.java > 3a38b24 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > 32a4044 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryServiceDiscovery.java > 7cbcc11 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestLeaderStatus.java > PRE-CREATION > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/ha/TestHaEnd2End.java > 07d74b5 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hive/AbstractTestWithStaticConfiguration.java > ced9d1c > > Diff: https://reviews.apache.org/r/48761/diff/ > > > Testing > ------- > > > Thanks, > > Colin McCabe > >
