> On April 17, 2018, 10:38 p.m., Sergio Pena wrote:
> > 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/diff/1/?file=2005071#file2005071line200>
> >
> > Isn't the oldContext.close() closing the leader monitor as well?
No. oldContext does not has reference of leader monitor. From the code below
from current version, you can see it only close curator framework
public void close() throws Exception {
this.curatorFramework.close();
}
> On April 17, 2018, 10:38 p.m., Sergio Pena wrote:
> > 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/diff/1/?file=2005071#file2005071line295>
> >
> > 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?
No. close() and resetHAContext() both calls this function
> On April 17, 2018, 10:38 p.m., Sergio Pena wrote:
> > 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/diff/1/?file=2005072#file2005072line187>
> >
> > 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.
first, we need to check if the input is null or not.
second, if it is null, leaderStatusMonitor is not null, but there will be null
exception when initialized the leaderStatusMonitor.
Thrire, when the system is shutdown, I need to get the monitor without knowing
the conf value. At the time, there should be an existing instance, so conf
value should not matter. Besides, I don't want to create an instance if there
is none
private static void closeLeaderStatusMonitor() throws Exception {
// make sure the leader status monitor is closed before closing curator
framework
LeaderStatusMonitor leaderMonitor =
LeaderStatusMonitor.getLeaderStatusMonitor(null);
if (leaderMonitor != null) {
leaderMonitor.close();
}
}
> On April 17, 2018, 10:38 p.m., Sergio Pena wrote:
> > 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/diff/1/?file=2005072#file2005072line217>
> >
> > 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?
deactivate() just sends a signal, so waiting for that signal in leader election
thread will stopping waiting, and then release leader lock. Releasing will
involve network communication. There is no way, as far as I know, to make sure
the leader lock is released.
as long as the lock release message is sent out, the lock should be released at
zookeeper. It is all local in-memory processing from the time leader election
thread gets the signal to sending out lock release message, so 5 ms in general
is enough. I can make it configurable, but it may be overkill. Or I can define
it as a constant in this class.
- Na
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66675/#review201354
-----------------------------------------------------------
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
>
>
