HBASE-24750 : All ExecutorService should use guava ThreadFactoryBuilder #2196
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🎊 +1 overall
This message was automatically generated. |
tedyu
reviewed
Aug 4, 2020
| Threads.newDaemonThreadFactory("rs(" + name + ")-backup")); | ||
| executor = new ThreadPoolExecutor(1, threads, keepAlive, TimeUnit.SECONDS, | ||
| new LinkedBlockingQueue<>(), | ||
| new ThreadFactoryBuilder().setNameFormat("rs(" + name + ")-backup-pool-%d").build()); |
Contributor
There was a problem hiding this comment.
Should the thread names follow existing format (dropping '-pool') ?
People may have got used to the current format during debugging.
Contributor
Author
There was a problem hiding this comment.
The reason why I appended -pool is to actually adhere with current format only. We add -pool in Threads class internally.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
ndimiduk
approved these changes
Aug 5, 2020
| return boundedCachedThreadPool; | ||
| } | ||
|
|
||
| public static ThreadPoolExecutor getBoundedCachedThreadPool(int maxCachedThread, long timeout, |
Member
There was a problem hiding this comment.
I wonder if these getBoundedCachedThreadPool methods could be replace to calls to methods directly on java.util.concurrent.Executors. I think we'd need to evaluate on a case-by-case basis how the resulting pools are being used and if they're constructed to correctly match the use-case.
Contributor
Author
There was a problem hiding this comment.
Agree, at least for now, let me make callers use getBoundedCachedThreadPool with ThreadFactoryBuilder input and only keep that version of getBoundedCachedThreadPool in Threads class.
hbase-server/src/test/java/org/apache/hadoop/hbase/procedure/SimpleRSProcedureManager.java
Outdated
Show resolved
Hide resolved
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
nyl3532016
reviewed
Aug 9, 2020
| Thread t = namedFactory.newThread(r); | ||
| if (handler != null) { | ||
| t.setUncaughtExceptionHandler(handler); | ||
| } else { |
Contributor
There was a problem hiding this comment.
@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.
compare with hbase-shaded-miscellaneous
private static ThreadFactory doBuild(ThreadFactoryBuilder builder) {
final String nameFormat = builder.nameFormat;
final Boolean daemon = builder.daemon;
final Integer priority = builder.priority;
final UncaughtExceptionHandler uncaughtExceptionHandler = builder.uncaughtExceptionHandler;
final ThreadFactory backingThreadFactory =
(builder.backingThreadFactory != null)
? builder.backingThreadFactory
: Executors.defaultThreadFactory();
final AtomicLong count = (nameFormat != null) ? new AtomicLong(0) : null;
return new ThreadFactory() {
@Override
public Thread newThread(Runnable runnable) {
Thread thread = backingThreadFactory.newThread(runnable);
if (nameFormat != null) {
thread.setName(format(nameFormat, count.getAndIncrement()));
}
if (daemon != null) {
thread.setDaemon(daemon);
}
if (priority != null) {
thread.setPriority(priority);
}
if (uncaughtExceptionHandler != null) {
thread.setUncaughtExceptionHandler(uncaughtExceptionHandler);
}
return thread;
}
};
}
Contributor
Author
There was a problem hiding this comment.
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 ThreadGroup takes care of it:
public void uncaughtException(Thread t, Throwable e) {
if (parent != null) {
parent.uncaughtException(t, e);
} else {
Thread.UncaughtExceptionHandler ueh =
Thread.getDefaultUncaughtExceptionHandler();
if (ueh != null) {
ueh.uncaughtException(t, e);
} else if (!(e instanceof ThreadDeath)) {
System.err.print("Exception in thread \""
+ t.getName() + "\" ");
e.printStackTrace(System.err);
}
}
}
However, if we want to maintain the same log present in LOGGING_EXCEPTION_HANDLER as default (which is what used to happen before this patch) behaviour, we should update the corresponding usages with uncaught exception handler as LOGGING_EXCEPTION_HANDLER in all places.
Let me raise a PR for this.
Contributor
There was a problem hiding this comment.
thank you for explaining about it.
virajjasani
added a commit
that referenced
this pull request
Aug 12, 2020
…er (#2214) Closes #2196 Signed-off-by: Nick Dimiduk <[email protected]> Signed-off-by: Ted Yu <[email protected]> Signed-off-by: niuyulin <[email protected]>
clarax
pushed a commit
to clarax/hbase
that referenced
this pull request
Nov 15, 2020
…er (apache#2214) Closes apache#2196 Signed-off-by: Nick Dimiduk <[email protected]> Signed-off-by: Ted Yu <[email protected]> Signed-off-by: niuyulin <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Jira: https://issues.apache.org/jira/browse/HBASE-24750