-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HBASE-24750 : All ExecutorService should use guava ThreadFactoryBuilder #2196
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,7 @@ | |
|
|
||
| import org.apache.hadoop.util.ReflectionUtils; | ||
| import org.apache.hadoop.util.StringUtils; | ||
| import org.apache.hbase.thirdparty.com.google.common.util.concurrent.ThreadFactoryBuilder; | ||
| import org.apache.yetus.audience.InterfaceAudience; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
@@ -197,83 +198,14 @@ public static ThreadPoolExecutor getBoundedCachedThreadPool( | |
| return boundedCachedThreadPool; | ||
| } | ||
|
|
||
| public static ThreadPoolExecutor getBoundedCachedThreadPool(int maxCachedThread, long timeout, | ||
| TimeUnit unit, String prefix) { | ||
| return getBoundedCachedThreadPool(maxCachedThread, timeout, unit, | ||
| newDaemonThreadFactory(prefix)); | ||
| } | ||
|
|
||
| /** | ||
| * Returns a {@link java.util.concurrent.ThreadFactory} that names each created thread uniquely, | ||
| * with a common prefix. | ||
| * @param prefix The prefix of every created Thread's name | ||
| * @return a {@link java.util.concurrent.ThreadFactory} that names threads | ||
| */ | ||
| public static ThreadFactory getNamedThreadFactory(final String prefix) { | ||
| SecurityManager s = System.getSecurityManager(); | ||
| final ThreadGroup threadGroup = (s != null) ? s.getThreadGroup() : Thread.currentThread() | ||
| .getThreadGroup(); | ||
|
|
||
| return new ThreadFactory() { | ||
| final AtomicInteger threadNumber = new AtomicInteger(1); | ||
| private final int poolNumber = Threads.poolNumber.getAndIncrement(); | ||
| final ThreadGroup group = threadGroup; | ||
|
|
||
| @Override | ||
| public Thread newThread(Runnable r) { | ||
| final String name = prefix + "-pool" + poolNumber + "-t" + threadNumber.getAndIncrement(); | ||
| return new Thread(group, r, name); | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Same as {#newDaemonThreadFactory(String, UncaughtExceptionHandler)}, | ||
| * without setting the exception handler. | ||
| */ | ||
| public static ThreadFactory newDaemonThreadFactory(final String prefix) { | ||
| return newDaemonThreadFactory(prefix, null); | ||
| } | ||
|
|
||
| /** | ||
| * Get a named {@link ThreadFactory} that just builds daemon threads. | ||
| * @param prefix name prefix for all threads created from the factory | ||
| * @param handler unhandles exception handler to set for all threads | ||
| * @return a thread factory that creates named, daemon threads with | ||
| * the supplied exception handler and normal priority | ||
| */ | ||
| 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 { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @virajjasani Sorry,I did not explain clear, I wonder if here may cause Inconsistent? In guava's builder class, if we do not assign UncaughtExceptionHandler, the UncaughtExceptionHandler will be null, not LOGGING_EXCEPTION_HANDLER
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. compare with hbase-shaded-miscellaneous
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, you are correct. We should not see issue from end result viewpoint if we are just executing some tasks and some exception interrupts executor thread specifically when However, if we want to maintain the same log present in
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thank you for explaining about it.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @nyl3532016 . |
||
| t.setUncaughtExceptionHandler(LOGGING_EXCEPTION_HANDLER); | ||
| } | ||
| if (!t.isDaemon()) { | ||
| t.setDaemon(true); | ||
| } | ||
| if (t.getPriority() != Thread.NORM_PRIORITY) { | ||
| t.setPriority(Thread.NORM_PRIORITY); | ||
| } | ||
| return t; | ||
| } | ||
|
|
||
| }; | ||
| } | ||
|
|
||
| /** Sets an UncaughtExceptionHandler for the thread which logs the | ||
| * Exception stack if the thread dies. | ||
| */ | ||
| public static void setLoggingUncaughtExceptionHandler(Thread t) { | ||
| t.setUncaughtExceptionHandler(LOGGING_EXCEPTION_HANDLER); | ||
| } | ||
|
|
||
| private static interface PrintThreadInfoHelper { | ||
| private interface PrintThreadInfoHelper { | ||
|
|
||
| void printThreadInfo(PrintStream stream, String title); | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the thread names follow existing format (dropping '-pool') ?
People may have got used to the current format during debugging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason why I appended
-poolis to actually adhere with current format only. We add-poolin Threads class internally.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tedyu could you please review?
Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change is fine.