HBASE-22634 : Improve performance of BufferedMutator #343
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
sbarnoud
changed the title
|
Is there something specific to branch-2.1 for this that makes it irrelevant or substantively different for the master branch? |
|
If i didn’t mistake BufferedMutatorImpl doesn’t exist anymore on master.
I build this on my cluster on a 2.1.4 ...
… Le 26 juin 2019 à 20:00, Sean Busbey ***@***.***> a écrit :
Is there something specific to branch-2.1 for this that makes it irrelevant or substantively different for the master branch?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
💔 -1 overall
This message was automatically generated. |
Apache9
requested changes
Jun 27, 2019
hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/AbstractRpcClient.java
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/AbstractRpcClient.java
Outdated
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/NettyRpcConnection.java
Show resolved
Hide resolved
Apache9
requested changes
Jun 27, 2019
hbase-client/src/main/java/org/apache/hadoop/hbase/client/BufferedMutatorImpl.java
Outdated
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncProcess.java
Outdated
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncRequestFutureImpl.java
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/client/SimpleRequestController.java
Outdated
Show resolved
Hide resolved
...e-client/src/main/java/org/apache/hadoop/hbase/client/BufferedMutatorThreadPoolExecutor.java
Show resolved
Hide resolved
Yes, it is gone on master. We reimplement the sync client based on async client on master so the BufferedMutator and AsyncProcess are both removed. |
|
💔 -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. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
A spark job with 7 executors, 4 cores each, has an average throughput of more than 500000 insert/s with optimal tuning of:
Where the input dataset is well balanced on the rowkey,on a table presplitted on 26 region servers, with a tiny average record size (500 bytes average). The source of the Spark job is as simple as: |
|
💔 -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. |
Apache9
requested changes
Aug 7, 2019
| @@ -235,6 +253,15 @@ public synchronized void close() throws IOException { | |||
| } | |||
| // Stop any running Periodic Flush timer. | |||
| disableWriteBufferPeriodicFlush(); | |||
| ap.waitAllSlot(); | |||
| try { | |||
| // Let time to the periodic flush thread to exit (task are finished, but not the code after) | |||
There was a problem hiding this comment.
What is the problem if the code after is not finished? I'm a bit nervous that we just set a magic 5ms sleep time here...
| @@ -93,6 +99,9 @@ | |||
| private final boolean cleanupPoolOnClose; | |||
| private volatile boolean closed = false; | |||
| private final AsyncProcess ap; | |||
| private List<AsyncRequestFuture> asfList; | |||
There was a problem hiding this comment.
Does this field need to be a class member? It seems that we only use it in the doFlush method.
| if (flushAll || writeBufferSize == 0) { | ||
| // if we have setWriteBufferPeriodicFlushTimeoutMs we may have concurrent update | ||
| List<AsyncRequestFuture> waitList; | ||
| synchronized(asfList) { |
There was a problem hiding this comment.
Oh, I see, BufferedMutator is thread safe, so multiple threads can enter this method at the same time and we want them to share the safe asfList, so it should be class member.
| } | ||
| if (nbRemoved == 0) { | ||
| try { | ||
| Thread.sleep(1); |
There was a problem hiding this comment.
So this means we will do a busy waiting here? This is not always the best choice, maybe we should provide a configurable way to wait here, the default one should be the typical wait/notify, and if do not care wasting the CPU cycles but only want the maximum throughput, you can use busy waiting.
| } else { | ||
| // Do some cleanup in asfList to decrease memory | ||
| int nbRemoved = 0; | ||
| while (asfList.size() >= maxThreads*4) { |
There was a problem hiding this comment.
Here we get size out of synchornized?
|
🎊 +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. |
|
🎊 +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. |
|
Close since no big progress here. Can open a new PR which targets to branch-2 if you have time @sbarnoud . Or I will try to port the patch if I have time. |
|
I’m on holiday.
Anyway, i’m just surprised you close a performance patch without doing any performance test.
So, i will keep the improvement for me ;-)
… Le 14 août 2019 à 08:24, Duo Zhang ***@***.***> a écrit :
Close since no big progress here. Can open a new PR which targets to branch-2 if you have time @sbarnoud . Or I will try to port the patch if I have time.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Performance is not everything. To be honest, the patch is still not qualified enough to be merged. Please see my last comments, doing busy wait( sleep(1) ) on a critical path is usually a no no, especially that you are waiting for external I/O to complete rather than an in memory operation... Anyway, I got your point that the current implementation of BufferMutator could be optimized to increase the concurrency. Will take a look if I have time. Thanks. |
|
The sleep is on a « backpressure » path. Of course, we can replace it with a notify.
But, when a system is overloaded it is often better to let it some time to unbuffer and not to overload it as soon as possible.
This kind of decision can only be taken after a benchmark and not just after a code review
… Le 14 août 2019 à 10:59, Duo Zhang ***@***.***> a écrit :
Performance is not everything. To be honest, the patch is still not qualified enough to be merged. Please see my last comments, doing busy wait( sleep(1) ) on a critical path is usually a no no, especially that you are waiting for external I/O to complete rather than an in memory operation...
Anyway, I got your point that the current implementation of BufferMutator could be optimized to increase the concurrency. Will take a look if I have time.
Thanks.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Not sure why you keep saying 'benchmark' since I never challenge the performance improvement of your patch? |
|
What i’m saying is that the sleep may provide better performance than a notify because it is called when the server latency increases (probably because it is overloaded).
In that case, it may be better to give some time to the server to dequeue queries instead of submitting as soon as possible new one.
Only a benchmark could differentiate the 2 approach, not a code review
… Le 14 août 2019 à 11:42, Duo Zhang ***@***.***> a écrit :
Not sure why you keep saying 'benchmark' since I never challenge the performance improvement of your patch?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Please read my comment carefully
I'm not telling that sleep will have a worse performance right? Just make it configurable. Maximum throughput is not always the best choice. User may have a mixed workload and do not want to waste too many CPU cycles here.
I can not get the point here, we must wait until the server dequeue queries right? Otherwise we do not need to introduce the complicated slot logic... Thanks. |
|
No 'while/sleep' nor 'wait/notify' needed anymore. |
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.
As requested in the Jira HBASE-22634