> 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?
I used AtomicBoolean so that closing LeaderStatus more than once only causes close() to run once. I'll add a comment. > 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? 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. > 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? 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. > 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 79 > > <https://reviews.apache.org/r/48761/diff/1/?file=1420479#file1420479line79> > > > > Nit: Thinking if there is a better name to not confuse this with number > > of active leaders. Some thing like becameLeaderCount? good idea > 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 97 > > <https://reviews.apache.org/r/48761/diff/1/?file=1420479#file1420479line97> > > > > Comment on these numbers? It's the retry policy for attempting to reconnect to ZooKeeper. This should probably be configurable eventually. The numbers basically determine the retry strategy. This strategy will sleep for approximately 2000, 4000. 8000 seconds (there is some randomness added) > 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 101 > > <https://reviews.apache.org/r/48761/diff/1/?file=1420479#file1420479line101> > > > > Define a constant for /leader suffix? OK > 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? 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. > 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 98 > > <https://reviews.apache.org/r/48761/diff/1/?file=1420480#file1420480line98> > > > > Is there a need for thread safety here? becomeActive and becomeStandby should never get called at the same time, so I don't believe there is a thread safety issue here. This code will change anyway to take some locks later since we will have to do more stuff to actually become active. > 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? 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. > On June 21, 2016, 12:55 p.m., Sravya Tirukkovalur wrote: > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestLeaderStatus.java, > > line 44 > > <https://reviews.apache.org/r/48761/diff/1/?file=1420485#file1420485line44> > > > > Just curious: Is 60 seconds really required? Or is it just a > > conservative upper bound? It's a conservative upper bound. We do not want tight bounds for tests, since then we get a lot of spurious failures when the test machine is under load. - Colin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48761/#review138824 ----------------------------------------------------------- On June 15, 2016, 10:17 p.m., Colin McCabe wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48761/ > ----------------------------------------------------------- > > (Updated June 15, 2016, 10:17 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 > >
