[
https://issues.apache.org/jira/browse/HBASE-25279?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17264634#comment-17264634
]
Viraj Jasani commented on HBASE-25279:
--------------------------------------
>From historical context,
One of the critical bugs to make ZK sync() sync, was fixed in HBASE-24603 and
it just went in 2.3.0, 2.2.6 releases. It's not been that long. However, with
this fix, [~bharathv] also fixed possible deadlock during ZK event processing
by introducing Single threaded executor pool, which would process event
notifications from ZK and thus event processing is now decoupled from ZK cncx
EventThread context. While doing so, our internal utility
Threads.getNamedThreadFactory() was used, and this was always clumsy, we never
had any proper pick for using ThreadFactory, some code used to get
ThreadFactory from our internal utility, some used guava libs. I wanted to
clean this up (IIRC, it was this very Jira HBASE-24603 in which I saw
differences of opinion for using ThreadFactory during review and I decided to
clean this up in HBASE-24750). HBASE-24750 had consensus to start using guava
only and get rid of our utility, which is what was done and now we only use
guava ThreadFactoryBuilder everywhere. However, I missed daemonizing threads
which led to bug HBASE-25037 , which [~zhangduo] fixed it quite sooner and we
are good there.
With respect to current Jira, I think Bharath didn't want to daemonize thread
used in Single threaded executor pool in the first place. It happened just
because our internal utility always used daemon mode. See *if (!t.isDaemon())
\{ t.setDaemon(true); }* here:
{code:java}
public static ThreadFactory newDaemonThreadFactory(final String prefix,
final UncaughtExceptionHandler handler) {
final ThreadFactory namedFactory = getNamedThreadFactory(prefix);
return new ThreadFactory() {
@Override
public Thread newThread(Runnable r) {
Thread t = namedFactory.newThread(r);
if (handler != null) {
t.setUncaughtExceptionHandler(handler);
} else {
t.setUncaughtExceptionHandler(LOGGING_EXCEPTION_HANDLER);
}
if (!t.isDaemon()) {
t.setDaemon(true);
}
if (t.getPriority() != Thread.NORM_PRIORITY) {
t.setPriority(Thread.NORM_PRIORITY);
}
return t;
}
};
}
{code}
This no longer exists in our code in the favour of using guava
ThreadFactoryBuilder.
[~zhangduo] Let me spend some time today and revisit HBASE-25037 to ensure if
we are not missing daemonizing any other place. If you could also cross verify
when you get time, that would be really great. WDYT?
> Non-daemon thread in ZKWatcher
> ------------------------------
>
> Key: HBASE-25279
> URL: https://issues.apache.org/jira/browse/HBASE-25279
> Project: HBase
> Issue Type: Bug
> Components: Zookeeper
> Reporter: Josh Elser
> Assignee: Josh Elser
> Priority: Critical
> Fix For: 3.0.0-alpha-1, 2.5.0, 2.4.1
>
>
> ZKWatcher spawns an ExecutorService which doesn't mark its threads as daemons
> which will prevent clean shut downs.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
