----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48761/#review138824 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatus.java (line 75) <https://reviews.apache.org/r/48761/#comment204026> May be comment on why is this an AtomicBoolean? Need for thread safety? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatus.java (lines 80 - 84) <https://reviews.apache.org/r/48761/#comment204015> Curious if there is an advantage of using rand here versus monotonically icnreasing id? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatus.java (lines 105 - 111) <https://reviews.apache.org/r/48761/#comment204016> 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? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusAdaptor.java (line 79) <https://reviews.apache.org/r/48761/#comment204018> Nit: Thinking if there is a better name to not confuse this with number of active leaders. Some thing like becameLeaderCount? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusAdaptor.java (line 97) <https://reviews.apache.org/r/48761/#comment204019> Comment on these numbers? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusAdaptor.java (line 101) <https://reviews.apache.org/r/48761/#comment204020> Define a constant for /leader suffix? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusAdaptor.java (line 163) <https://reviews.apache.org/r/48761/#comment204022> What is the behavior if becomeStandby fails? There might be more than one leader? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java (line 98) <https://reviews.apache.org/r/48761/#comment204027> Is there a need for thread safety here? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java (line 170) <https://reviews.apache.org/r/48761/#comment204023> Is the comment still relevant? sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestLeaderStatus.java (line 44) <https://reviews.apache.org/r/48761/#comment204024> Just curious: Is 60 seconds really required? Or is it just a conservative upper bound? Thanks for the patch Colin! Overall approach looks good to me, I still did not finish reviewing the test cases though. Left some minor comments meanwhile. - Sravya Tirukkovalur 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 > >
