HBASE-24436 The store file open and close thread pool should be share… #1783
+60
−50
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. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
apurtell
requested changes
May 26, 2020
| private RegionCoprocessorHost coprocessorHost; | ||
|
|
||
| private TableDescriptor htableDescriptor = null; | ||
| private ThreadPoolExecutor storeFileOpenAndCloseThreadPool; |
There was a problem hiding this comment.
Why is this shared at the Region level. Shouldn't it be at the Store level (in HStore)?
What does sharing at the region level gain us? Are you attempting to evenly round-robin store open work over all opening stores in the region? Just sharing an executor at region level won't do this. If the underlying stores are skewed the order in which runnables are submitted to the executor will share the skew.
There was a problem hiding this comment.
If I understand the jira correctly, what you are trying to solve is below case.
One region with say 2 stores. Store1 having much more files than other. Say the config for the #threads in open pool is 10. Now it will create 2 pools for each store with 5 threads each. The Store2 will get finished soon. But store1 will take much longer. So if it was a shared pool of 10 threads the overall time for opening both stores would have been lesser. my understanding correct?
There was a problem hiding this comment.
If I understand the jira correctly, what you are trying to solve is below case.
One region with say 2 stores. Store1 having much more files than other. Say the config for the #threads in open pool is 10. Now it will create 2 pools for each store with 5 threads each. The Store2 will get finished soon. But store1 will take much longer. So if it was a shared pool of 10 threads the overall time for opening both stores would have been lesser. my understanding correct?
Yes, just that case.
There was a problem hiding this comment.
Why is this shared at the Region level. Shouldn't it be at the Store level (in HStore)?
What does sharing at the region level gain us? Are you attempting to evenly round-robin store open work over all opening stores in the region? Just sharing an executor at region level won't do this. If the underlying stores are skewed the order in which runnables are submitted to the executor will share the skew.
If only one store file is too slow and that is the bottleneck of the whole open or close region process, it seems not easy to fix that by threadpool here(maybe hedged read?). As Anoops said above, the problem we want to solve here is some stores may have too many store files, while others have much fewer. It will be quicker ideally if all of the store files just share the whole thread pool in one region.
|
💔 -1 overall
This message was automatically generated. |
anoopsjohn
reviewed
May 27, 2020
| } | ||
| } | ||
|
|
||
| storeFileOpenAndCloseThreadPool.shutdownNow(); |
There was a problem hiding this comment.
This means keeping this 'storeFileOpenAndCloseThreadPool' active till the Region is closed.
previously we create the pool at the region open time and once the stores are opened, the pool is shutdown. Keeping it longer this way not looks good
There was a problem hiding this comment.
This means keeping this 'storeFileOpenAndCloseThreadPool' active till the Region is closed.
previously we create the pool at the region open time and once the stores are opened, the pool is shutdown. Keeping it longer this way not looks good
There is a timeout for this thread pool, after some time, the threads will be destroyed.
There was a problem hiding this comment.
The non core threads will get removed after the idle timeout. But the way we create the pool, we pass the number of threads (as per the config) as the core threads count.
But we set
boundedCachedThreadPool.allowCoreThreadTimeOut(true)
So this helps. I missed this part.
| completionService.submit(() -> { | ||
| Thread t = Thread.currentThread(); | ||
| String name = t.getName(); | ||
| Thread.currentThread().setName(executingThreadNamePrefix + "-" |
There was a problem hiding this comment.
This is done to dynamically change the thread name as per the Store name?
| completionService.submit(new Callable<Void>() { | ||
| @Override | ||
| public Void call() throws IOException { | ||
| Thread t = Thread.currentThread(); |
There was a problem hiding this comment.
All these looks bit hacky
There was a problem hiding this comment.
All these looks bit hacky
Changing the thread name is not necessary, but it will be useful when jstack the process, we will know something in detail.Yeah, it's bit hacky
There was a problem hiding this comment.
All these looks bit hacky
What do you think about here, Anoop? Just remove the name related logic? Or just set and not reset ?
There was a problem hiding this comment.
I dont think we should do these hacks. The threads can be named with regionName and some seqNo. Anyway even if we do these hacks and add the cf name, it says till that level only. Still from jstack we can not know which file open it is stuck with (if assume we took it when there was a stuck case). We can make sure we have proper log which says opening store file xxx for cf x and that log will have to say the thread name also. So later with jstack and this log we can track down issues. Sounds ok?
There was a problem hiding this comment.
I dont think we should do these hacks. The threads can be named with regionName and some seqNo. Anyway even if we do these hacks and add the cf name, it says till that level only. Still from jstack we can not know which file open it is stuck with (if assume we took it when there was a stuck case). We can make sure we have proper log which says opening store file xxx for cf x and that log will have to say the thread name also. So later with jstack and this log we can track down issues. Sounds ok?
Yeah, definitely, let me update the patch soon.
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
saintstack
approved these changes
May 28, 2020
| } | ||
| } finally { | ||
| storeFileOpenerThreadPool.shutdownNow(); | ||
|
|
There was a problem hiding this comment.
If nothing to cleanup, remove the finally block?
There was a problem hiding this comment.
If nothing to cleanup, remove the finally block?
Ok, let me update the patch a moment later
|
Can't commit till the -1 from JIRA is raised. |
Thanks, sir, I am preparing for the data |
ArthurSXL8
force-pushed
the
HBASE-24436
branch
3 times, most recently
from
ca3f80d to
4cf98cd
Compare
|
💔 -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. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
ArthurSXL8
force-pushed
the
HBASE-24436
branch
2 times, most recently
from
fc31124 to
3617561
Compare
|
💔 -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. |
|
💔 -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. |
|
💔 -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. |
…d at the region level
|
💔 -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. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 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.
…d at the region level