----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66675/#review201354 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java Line 200 (original), 200-202 (patched) <https://reviews.apache.org/r/66675/#comment282531> Isn't the oldContext.close() closing the leader monitor as well? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java Lines 295-299 (patched) <https://reviews.apache.org/r/66675/#comment282532> If the close() method that calls closeLeaderStatusMonitor() is the only method that will close the context and monitor, should we consider on putting this code on the close() method instead? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/LeaderStatusMonitor.java Line 187 (original), 187 (patched) <https://reviews.apache.org/r/66675/#comment282533> Why the conf parameter can be null? I saw that in an earlier comment, but I wasn't sure why we need to pass a conf null parameter. Btw, if the conf is null, then the leaderStatusMonitor will be null. I don't think this method should be called with conf as null. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/LeaderStatusMonitor.java Lines 217 (patched) <https://reviews.apache.org/r/66675/#comment282534> This is a magic number that can varies for different circustances. What if it takes more than 5s to be deactivated? Isn't there something to check when the leader was deactivated completely? sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestLeaderStatusMonitor.java Lines 202 (patched) <https://reviews.apache.org/r/66675/#comment282537> Can you add some javadoc like the above methods do that explain this test case? sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestLeaderStatusMonitor.java Lines 244-268 (patched) <https://reviews.apache.org/r/66675/#comment282538> Isn't this test case already done in testTwoMonitors? - Sergio Pena On April 17, 2018, 10:06 p.m., Na Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66675/ > ----------------------------------------------------------- > > (Updated April 17, 2018, 10:06 p.m.) > > > Review request for sentry, Alexander Kolbasov, Arjun Mishra, and Sergio Pena. > > > Repository: sentry > > > Description > ------- > > In sentry HA setup, When curator framework is closed before leader lock is > released, after sentry service restart without restarting zookeeper, sentry > servers cannot get leader lock, and therefore there will be no leader in > sentry to sync with HMS. > The fix is to control the shutdown order and make sure the leader lock is > released before curator framework is closed. > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/HAContext.java > 71865ca > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/LeaderStatusMonitor.java > 0a208d4 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestLeaderStatusMonitor.java > 395516c > > > Diff: https://reviews.apache.org/r/66675/diff/1/ > > > Testing > ------- > > add Unit test case to verify that the exception "ERROR > sentry.org.apache.curator.framework.recipes.leader.LeaderSelector: The leader > threw an exception java.lang.IllegalStateException: instance must be started > before calling this method" does not happen > > > Thanks, > > Na Li > >
