Uh oh!
There was an error while loading. Please reload this page.
HBASE-26468 Region Server doesn't exit cleanly incase it crashes. - #3862
Merged
Conversation
Apache-HBase
commented
Nov 19, 2021
🎊 +1 overall
This message was automatically generated. |
virajjasani
commented
Nov 19, 2021
Contributor
This looks good actually, just wondering what could be the most destructive action that non-daemons could do. Maybe nothing? because regionserver/master is already (or in the progress of) shutdown? |
Apache-HBase
commented
Nov 19, 2021
🎊 +1 overall
This message was automatically generated. |
shahrs87
commented
Nov 19, 2021
ContributorAuthor
It can't do anything since every interaction with hmaster/regionserver will fail. |
shahrs87
commented
Nov 19, 2021
ContributorAuthor
If the control reaches in the changed code, then HMaster/HRegionServer thread has already died. |
virajjasani
commented
Nov 19, 2021
Contributor
Yeah that sounds right, server would have already been unresponsive. Also, nothing happens after call to |
Apache-HBase
commented
Nov 19, 2021
🎊 +1 overall
This message was automatically generated. |
Apache9
commented
Nov 20, 2021
Contributor
The comment of the method says it will only call System.exit when a nonzero return code is returned from the run method? |
shahrs87
commented
Nov 20, 2021
ContributorAuthor
@Apache9 good catch. Changed the comment. Please review again. |
virajjasani
commented
Nov 20, 2021
Contributor
Apache-HBase
commented
Nov 20, 2021
🎊 +1 overall
This message was automatically generated. |
Apache9
commented
Nov 20, 2021
Contributor
What I mean is that the method is design to be like the old behavior, so we need to make sure that it is OK for us to change the behavior. We could fix a bug introduced by the old behavior but we may introduce more bugs if we change the behavior... Of course I'm not telling that we can not apply the patch, but we need to take more care. Thanks. |
Apache-HBase
commented
Nov 20, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Nov 20, 2021
🎊 +1 overall
This message was automatically generated. |
shahrs87
commented
Nov 20, 2021
ContributorAuthor
@Apache9 Agree with what you are saying. I don't see how this patch introduces any more bugs. All the tests also succeeded. Do you see any concern in this patch ? |
Apache9
commented
Nov 21, 2021
Contributor
The code is pretty old... This is the commit, a refactoring actually, and there is a comment So I think we do not call System.exit when ret is zero is intentional. A zero return value means a normal shutdown, so here we want all the threads clean themselves up and then exit gracefully. The problem here is that, why a crashed region server could return zero here? Why it does not return a non zero value? Sorry for sticking on the behavior here, the modification is pretty simple but it is very foundamental... Thanks. |
virajjasani
commented
Nov 22, 2021
Contributor
@Apache9 Regionserver doesn't crash, it exits normally. It prints log: |
virajjasani
commented
Nov 22, 2021
Contributor
Also, now I see that @shahrs87 has also updated the description of the Jira HBASE-26468 and explained what exactly goes wrong and why server stays frozen if one of the non-daemon threads is not terminated. |
ContributorAuthor
I have added all the details in HBASE-26468 but adding here for the context. Within RS (atleast in branch-1.6) there is a subtle difference when RS is aborted vs when it is stopped. In this particular case, the RS was stopped. In HRegionServerCommandLine, if a server is aborted, then it throws RTE which in turns converts into System.exit(1). But if RS is stopped, it returns return code of 0. Sure we can make changes to this code to return -1 when RS is stopped so it behaves exactly alike when it is aborted. But there will be bugs in future when RS will exit with return code 0 when it should have return with non zero exit code.
We can always wait for few seconds for threads to clean themselves up if RS exits with 0 return code. But we shouldn't wait infinitely for threads to clean themselves up.
Don't be sorry. It is a good discussion and we should always try to understand the implications before we make any fundamental changes. |
Apache-HBase
commented
Nov 25, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Nov 25, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Nov 25, 2021
🎊 +1 overall
This message was automatically generated. |
virajjasani
left a comment
Contributor
There was a problem hiding this comment.
Left a few comments, the approach looks good overall. Nice one! And thanks to you too @Apache9 for the suggestion
Uh oh!
There was an error while loading. Please reload this page.
| Thread.sleep(1000); | ||
| } | ||
| if (forceStop) { | ||
| LOG.error("Failed to stop all non-daemon threads, so force quitting"); |
Contributor
There was a problem hiding this comment.
WARN or FATAL log level might be more suitable
ContributorAuthor
There was a problem hiding this comment.
The process has already stopped/aborted so I don't want to log it at FATAL. So I thought ERROR is more appropriate. Kept it at ERROR. Please let me know if you feel v strong against ERROR.
Contributor
There was a problem hiding this comment.
ERROR seems good 👍
How about Failed to stop all non-daemon threads, so terminating the regionserver JVM ?
ContributorAuthor
There was a problem hiding this comment.
Removed regionserver word from above message since this is common to HMaster also.
| // Exclude current thread | ||
| if (t.getId() != Thread.currentThread().getId() && !t.isDaemon()) { | ||
| nonDaemonThreadCount.getAndIncrement(); | ||
| LOG.info("Non daemon thread name: " + t.getName() + " still alive"); |
Contributor
There was a problem hiding this comment.
It will be good if we could print the stack trace here.
ContributorAuthor
There was a problem hiding this comment.
Done. Please review again.
shahrs87
commented
Nov 26, 2021
ContributorAuthor
Sample log lines after deploying this code to internal test cluster |
shahrs87
commented
Nov 26, 2021
| if (t.getId() != Thread.currentThread().getId() && !t.isDaemon()) { | ||
| nonDaemonThreadCount.getAndIncrement(); | ||
| LOG.info("Non daemon thread {} is still alive", t.getName()); | ||
| LOG.info(printStackTrace(t)); |
ContributorAuthor
There was a problem hiding this comment.
This is the sample stack trace after deploying it to internal cluster
2021-11-26 19:21:16,747 INFO [main] util.Threads - Non daemon thread main.named-queue-events-pool-pool1-t1 is still alive
2021-11-26 19:21:16,748 INFO [main] util.Threads - sun.misc.Unsafe.park(Native Method)
java.util.concurrent.locks.LockSupport.park(LockSupport.java:175)
java.util.concurrent.locks.AbstractQueuedSynchronizer$ConditionObject.await(AbstractQueuedSynchronizer.java:2044)
com.lmax.disruptor.BlockingWaitStrategy.waitFor(BlockingWaitStrategy.java:45)
com.lmax.disruptor.ProcessingSequenceBarrier.waitFor(ProcessingSequenceBarrier.java:56)
com.lmax.disruptor.BatchEventProcessor.run(BatchEventProcessor.java:124)
java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
java.lang.Thread.run(Thread.java:748)
There is no easy method to print stack trace given a thread handle. Thread#dumpStack dumps stack of current thread but we don't need that.
Cc @virajjasani@Apache9
Apache-HBase
commented
Nov 26, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Nov 26, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Nov 27, 2021
🎊 +1 overall
This message was automatically generated. |
shahrs87
commented
Nov 29, 2021
ContributorAuthor
@virajjasani@Apache9 Can you please review the patch again. I tested the change on internal test cluster and it works as expected. Thank you ! |
virajjasani
left a comment
Contributor
There was a problem hiding this comment.
Left 2 minor nits, +1 overall
Comment on lines
+227
to
+228
| sb.append("\n"); | ||
| sb.append(" " + frame.toString()); |
Contributor
There was a problem hiding this comment.
nit: sb.append("\n").append(" ").append(frame.toString())
Apache-HBase
commented
Nov 29, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Nov 29, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Nov 30, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Nov 30, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Nov 30, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Nov 30, 2021
🎊 +1 overall
This message was automatically generated. |
virajjasani
approved these changes
Nov 30, 2021
| public static String printStackTrace(Thread t) { | ||
| StringBuilder sb = new StringBuilder(); | ||
| for (StackTraceElement frame: t.getStackTrace()) { | ||
| sb.append("\n").append(" " + frame.toString()); |
Contributor
There was a problem hiding this comment.
nit: append(" ").append(frame.toString())
ContributorAuthor
There was a problem hiding this comment.
Kept multiple spaces so as to make it look like stack trace.
Contributor
There was a problem hiding this comment.
Oh no not that, I meant adding frame.toString() with .append() rather than String concat. But it's minor nit only. Maybe you can make this change (or not) and also keep backport PRs (#3865#3866) up-to date, and all can be merged together tomorrow.
(I will take care of branch-2.4 backport from branch-2 backport PR)
Apache9
approved these changes
Nov 30, 2021
Apache-HBase
commented
Nov 30, 2021
🎊 +1 overall
This message was automatically generated. |
shahrs87
commented
Nov 30, 2021
ContributorAuthor
@virajjasani Updated the current PR and backports for branch-2 and branch-1 |
Apache-HBase
commented
Nov 30, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Nov 30, 2021
🎊 +1 overall
This message was automatically generated. |
virajjasani pushed a commit
that referenced
this pull request
Dec 1, 2021
) (#3862) Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Geoffrey Jacoby <gjacoby@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org>
virajjasani pushed a commit
that referenced
this pull request
Dec 1, 2021
) (#3862) Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Geoffrey Jacoby <gjacoby@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org>
virajjasani pushed a commit
that referenced
this pull request
Dec 1, 2021
) (#3862) Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Geoffrey Jacoby <gjacoby@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org>
guangxuCheng pushed a commit
to guangxuCheng/hbase
that referenced
this pull request
Mar 28, 2022
vinayakphegde pushed a commit
to vinayakphegde/hbase
that referenced
this pull request
Apr 4, 2024
…ache#3865) (apache#3862) Signed-off-by: Duo Zhang <zhangduo@apache.org> Signed-off-by: Geoffrey Jacoby <gjacoby@apache.org> Signed-off-by: Viraj Jasani <vjasani@apache.org>
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
Sign up for freeto 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.
No description provided.