HBASE-22867 The ForkJoinPool in CleanerChore will spawn thousands of threads in our cluster with thousands table #513
Conversation
This file contains 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
|
PR an initial patch, need more consideration.. |
|
💔 -1 overall
This message was automatically generated. |
Apache9
requested changes
Aug 20, 2019
| task.fork(); | ||
| } | ||
| allSubdirsDeleted = deleteAction(() -> getCleanResult(tasks), "subdirs"); | ||
| subDirs.forEach(dir -> futures.add(pool.execute(new CleanerTask(dir, false)))); |
There was a problem hiding this comment.
I think submitting the task to the same pool is not safe here. In ForkJoinPool, it is safe to call join as in the join method, the current thread will try to execute tasks if there are any, so in general, we can always make progress and finally finish all the tasks. But here, the current thread will be blocked on the future.get, so it is possible that all the threads in the pool are blocked on a future.get and cause dead lock here.
|
Ping @Apache9 , I've rewriten the cleaner , please take a look again. |
|
Seems still some problem, the UT are easy to hang now, Let me check. |
|
OK, I got the problem, forget to complete the future somewhere.. it's easy to fix . |
|
Fixed all the UT in cleanerChore, should be good now. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Apache9
requested changes
Aug 21, 2019
There was a problem hiding this comment.
In general I think this is a possible way to solve the problem. Using CompletableFuture to get the final result, and in the actions which will be executed in thread pool, just return without wating for the result of the CompletableFuture. But seems we still use the ForkJoinPool? This may still lead to too many threads?
| // Step.4: Once all sub-files & sub-directories are deleted, then can try to delete the | ||
| // current directory asynchronously. | ||
| CompletableFuture.allOf(futures.toArray(new CompletableFuture[futures.size()])) | ||
| .whenComplete((voidObj, e) -> { |
There was a problem hiding this comment.
Better use FutureUtils.addListener here. Although we do not run error-prone checks now in pre commit, this will lead to a FutureReturnValueIgnored warning, and it is a problem. If the code in whenComplete throw an exception, the current implementation will hide it and you can only see the program hangs, using FutureUtils.addListener can print out the exception stacktrace at least.
There was a problem hiding this comment.
OK, let me update the patch.
@Apache9 |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Ping @Apache9 @Reidddddd any other concerns ? I've made this working in our offline cluster (which encountered this problem before), it seems work pretty good now. |
|
🎊 +1 overall
This message was automatically generated. |
Apache9
requested changes
Aug 21, 2019
| @@ -213,9 +213,14 @@ private void preRunCleaner() { | |||
|
|
|||
| public Boolean runCleaner() { | |||
There was a problem hiding this comment.
Can be boolean here? We will not return null in the future.get?
| } catch (Exception ie) { | ||
| // Must handle the inner exception here, otherwise the result may get stuck if one | ||
| // sub-directory get some failure. | ||
| result.completeExceptionally(ie); |
There was a problem hiding this comment.
I think in the past we will return false instead of throwing the exception out?
There was a problem hiding this comment.
Em, let me check whether it will impact the behavior ..
There was a problem hiding this comment.
I've checked the logic, should have no difference here. Both the exeption from sub-directory & boolean flag will skip the deletion of current directory, should be OK.
| synchronized void execute(ForkJoinTask<?> task) { | ||
| pool.execute(task); | ||
| synchronized void runAsync(Runnable runnable) { | ||
| CompletableFuture.runAsync(runnable, pool); |
There was a problem hiding this comment.
Emmm, what is this used for? The method will return a CompletableFuture but we just ignore it? Then what is the difference comparing to just call pool.execute?
There was a problem hiding this comment.
Fine, can just use the pool#execute now. plan to use the CompletableFuture of runAsync to get the final result befoe.
| LOG.info("Update chore's pool size from {} to {}", pool.getParallelism(), size); | ||
| pool = new ForkJoinPool(size); | ||
| LOG.info("Update chore's pool size from {} to {}", pool.getPoolSize(), size); | ||
| pool = initializePool(size); |
There was a problem hiding this comment.
For ThreadPoolExecutor, there is a setCorePoolSize method so we do not need to recreate it. I think we can make it as a final member.
There was a problem hiding this comment.
OK, let me address this in the new patch, good suggestion.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
openinx
force-pushed
the
master
branch
2 times, most recently
from
1483af2 to
a3b3826
Compare
Apache9
requested changes
Aug 22, 2019
| LOG.info("Cleaner pool size is {}", size); | ||
| cleanerLatch = 0; | ||
| } | ||
|
|
||
| private static ThreadPoolExecutor initializePool(int size) { | ||
| return new ThreadPoolExecutor(size, size, 500, TimeUnit.SECONDS, new LinkedBlockingQueue<>(), |
There was a problem hiding this comment.
Usually it will be 1 minute? Why 500 seconds? And I think we can call allowCoreThreadTimeOut(true) here to release the threads if not needed?
There was a problem hiding this comment.
OK, did not consider much about the keepAliveTime (can make it 1min if neccessary). the allowCoreThreadTimeOut sounds good.
|
💔 -1 overall
This message was automatically generated. |
|
The hbase QA seems is unrelated , but will try the HBase QA again. |
Reidddddd
reviewed
Aug 22, 2019
| } | ||
| }); | ||
| } catch (Exception e) { | ||
| LOG.warn("Failed to traverse and delete the path: {}", dir, e); |
There was a problem hiding this comment.
LOG.warn here may be very chatty. Debug or Trace is up to your choice.
There was a problem hiding this comment.
OK, here can be DEBUG because we will put the exception in result, and will get it in runCleaner method. Let me fix this.
Reidddddd
requested changes
Aug 22, 2019
Reidddddd
requested changes
Aug 22, 2019
Reidddddd
approved these changes
Aug 22, 2019
…threads in our cluster with thousands table
asfgit
pushed a commit
that referenced
this pull request
Aug 22, 2019
…threads in our cluster with thousands table (#513) Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Reid Chan <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
Aug 22, 2019
…threads in our cluster with thousands table (#513) Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Reid Chan <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
Aug 22, 2019
…threads in our cluster with thousands table (#513) Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Reid Chan <[email protected]>
|
💔 -1 overall
This message was automatically generated. |
symat
pushed a commit
to symat/hbase
that referenced
this pull request
Feb 17, 2021
…threads in our cluster with thousands table (apache#513) Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Reid Chan <[email protected]> (cherry picked from commit 0690864) Change-Id: Ic4fcf6c8c5d585da2f65e881bee500149a3c1ace
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
…threads in our cluster with thousands table