----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48761/#review138965 -----------------------------------------------------------
Thanks Colin, great work! Leaved some comments. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatus.java (line 33) <https://reviews.apache.org/r/48761/#comment204153> Add "It handles both the HA and non-HA case.", to be more specific in the comment? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatus.java (line 127) <https://reviews.apache.org/r/48761/#comment204161> Should we specify the sentry server name in the log? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java (line 160) <https://reviews.apache.org/r/48761/#comment204162> log the sentry server name? same for line 166. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java (line 170) <https://reviews.apache.org/r/48761/#comment204157> Probably move the start into call? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java (line 306) <https://reviews.apache.org/r/48761/#comment204163> Should leaderstatus.close() be called here? sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestLeaderStatus.java (line 181) <https://reviews.apache.org/r/48761/#comment204170> Should we start a sentry server here? Or we just testing a general server? - Hao Hao On June 21, 2016, 4:18 p.m., Colin McCabe wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48761/ > ----------------------------------------------------------- > > (Updated June 21, 2016, 4:18 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 > >
