----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55190/#review160624 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java (line 67) <https://reviews.apache.org/r/55190/#comment231840> This should be made final for consistency. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java (line 97) <https://reviews.apache.org/r/55190/#comment231763> Nit: I don't think you need Arrays.asList() here - simple 'for (String principal : allowConnect.split(...))' should work. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java (line 123) <https://reviews.apache.org/r/55190/#comment231765> The "use ..." javadoc for the "get..." method sounds a bit incoherent. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java (line 149) <https://reviews.apache.org/r/55190/#comment231769> Make this a proper javadoc please? Also, is it an established naming scheme, where "HAServerContext" means a HAContext with ACLs checked? Maybe getSecureHAContext() or some such? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java (line 170) <https://reviews.apache.org/r/55190/#comment231838> Does this code have an official line length limit? Usually there is some, like 100 characters. This line and several others in this method look very long, which would make working with them harder in the future. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java (line 186) <https://reviews.apache.org/r/55190/#comment231839> Nit: useless empty line. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java (line 191) <https://reviews.apache.org/r/55190/#comment231841> Nit: I think indentation everywhere in this file is 2 characters. Please be consistent. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java (line 239) <https://reviews.apache.org/r/55190/#comment231842> Nit: indentation here and in the next method. - Misha Dmitriev On Jan. 4, 2017, 10:51 p.m., Alexander Kolbasov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55190/ > ----------------------------------------------------------- > > (Updated Jan. 4, 2017, 10:51 p.m.) > > > Review request for sentry, Colin Ma, Misha Dmitriev, Dapeng Sun, Hao Hao, > kalyan kumar kalvagadda, Vamsee Yarlagadda, and Vadim Spector. > > > Bugs: SENTRY-1583 > https://issues.apache.org/jira/browse/SENTRY-1583 > > > Repository: sentry > > > Description > ------- > > - Restored HAContext class > - Removed Fencing support from HA > - Removed Activator, Activators, and LeaderStatus classes > - Renamed LeaderStatusAdaptor to LeaderStatusMonitor which is now the sole > class responsible for leader status monitoring. > > > Diffs > ----- > > pom.xml df26edf74c04fffcd9fbc3da07e949362eb94728 > > sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/SentryConstants.java > c094058142706a6b564c54fa69ddff5f1e2e5048 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryMetrics.java > f80605c4d68ef266a24e65d14f51066388b48417 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/Activator.java > c3df4d8a6568f236dc5e05ab190cd1f484f7967a > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/Activators.java > 2926eeb97ec450f12d0d08086165c091709ead99 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatus.java > 2467921b3cdc9ebd267d308d5cdcce52a836f207 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusAdaptor.java > e87b3d13dcf2d10eb7ef00d21b8fa8846b4d16c0 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/LeaderStatusMonitor.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/SentryService.java > a9d49f86961f1ced1af40afcfe6172f884a23d44 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/service/thrift/ServiceConstants.java > d80fa1e71c165b7f1faf1c50c451e049d76b770b > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestActivator.java > 60cfc735832433fb4dbeae1c2d617dd713fc3f3e > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceMetrics.java > bc375e32685dcbcbf0b6398856dceced3de789e6 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/service/thrift/TestLeaderStatus.java > c796fabc21005479fa6afc687cf1df7af76538a9 > > Diff: https://reviews.apache.org/r/55190/diff/ > > > Testing > ------- > > Performed manual testing with two instances of Sentry running. Tried the > following actions: > > - Sending SIGUSR2 to processes which changes leadership status (verified via > log messages and emtrics) > - Killing active server - new sserver is elected as a leader > - Restarting ZK service - a leader is elected once ZK service is back. > > > Thanks, > > Alexander Kolbasov > >
