You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@bookkeeper.apache.org by "hangc0276 (via GitHub)" <gi...@apache.org> on 2023/03/27 13:49:37 UTC
[GitHub] [bookkeeper] hangc0276 opened a new pull request, #3889: [improve] [client] Group read/write response when jumping to the ordered executor
hangc0276 opened a new pull request, #3889:
URL: https://github.com/apache/bookkeeper/pull/3889
### Motivation
When the PerChannelBookieClient receives read/write responses from the Bookie server, it processes those responses one by one with the following pipeline
- Get the CompletionValue according to the CompletionKey
- Jump to the ordered executor the run the callback
https://github.com/apache/bookkeeper/blob/cbd634dbe0c894474a934fab6d8711588ad94114/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java#L1353-L1374
If there are 1000 responses, we need to jump 1000 times into the ordered executor and cause 1000 times thread context switches.
### Design
We can group those responses according to whether the Netty socket channel is empty. We initiate a Blocking Queue to buffer all the responses, and the default capacity is 1_000. There are two conditions that will trigger the response process.
- The Blocking queue is full
- The Netty socket channel is empty and the `channelReadComplete` is called.
If we buffered 1_000 responses in the queue, we will reduce 1_000 times thread context switch if those responses are hashed to the same thread to execute.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hangc0276 commented on a diff in pull request #3889: [improve] [client] Group read/write response when jumping to the ordered executor
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 commented on code in PR #3889:
URL: https://github.com/apache/bookkeeper/pull/3889#discussion_r1155055335
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java:
##########
@@ -173,6 +178,7 @@ public class PerChannelBookieClient extends ChannelInboundHandlerAdapter {
BKException.Code.DuplicateEntryIdException,
BKException.Code.WriteOnReadOnlyBookieException));
private static final int DEFAULT_HIGH_PRIORITY_VALUE = 100; // We may add finer grained priority later.
+ private static final int DEFAULT_CAPACITY = 1_000;
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] dlg99 commented on a diff in pull request #3889: [improve] [client] Group read/write response when jumping to the ordered executor
Posted by "dlg99 (via GitHub)" <gi...@apache.org>.
dlg99 commented on code in PR #3889:
URL: https://github.com/apache/bookkeeper/pull/3889#discussion_r1152790493
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java:
##########
@@ -1341,7 +1349,10 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception
if (msg instanceof BookieProtocol.Response) {
BookieProtocol.Response response = (BookieProtocol.Response) msg;
- readV2Response(response);
+ if (!responses.offer(response)) {
Review Comment:
It will increase the latency of the latency-sensitive workloads.
At least this has to be behind a feature flag.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] zymap commented on a diff in pull request #3889: [improve] [client] Group read/write response when jumping to the ordered executor
Posted by "zymap (via GitHub)" <gi...@apache.org>.
zymap commented on code in PR #3889:
URL: https://github.com/apache/bookkeeper/pull/3889#discussion_r1150250981
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java:
##########
@@ -326,6 +332,7 @@ public class PerChannelBookieClient extends ChannelInboundHandlerAdapter {
volatile Channel channel = null;
private final ClientConnectionPeer connectionPeer;
private volatile BookKeeperPrincipal authorizedId = BookKeeperPrincipal.ANONYMOUS;
+ private final BlockingQueue<BookieProtocol.Response> responses;
Review Comment:
Maybe we can use BatchedArrayBlockingQueue? Then we can use takeAll() to get better performance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve] [client] Group read/write response when jumping to the ordered executor [bookkeeper]
Posted by "ytong01 (via GitHub)" <gi...@apache.org>.
ytong01 commented on PR #3889:
URL: https://github.com/apache/bookkeeper/pull/3889#issuecomment-1812192184
I see this is only for v2, it does not work for v3?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hangc0276 commented on a diff in pull request #3889: [improve] [client] Group read/write response when jumping to the ordered executor
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 commented on code in PR #3889:
URL: https://github.com/apache/bookkeeper/pull/3889#discussion_r1155055313
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java:
##########
@@ -1341,7 +1349,10 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception
if (msg instanceof BookieProtocol.Response) {
BookieProtocol.Response response = (BookieProtocol.Response) msg;
- readV2Response(response);
+ if (!responses.offer(response)) {
Review Comment:
Good point. I add a flag to enable this feature.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] horizonzy commented on a diff in pull request #3889: [improve] [client] Group read/write response when jumping to the ordered executor
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy commented on code in PR #3889:
URL: https://github.com/apache/bookkeeper/pull/3889#discussion_r1150951754
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java:
##########
@@ -173,6 +178,7 @@ public class PerChannelBookieClient extends ChannelInboundHandlerAdapter {
BKException.Code.DuplicateEntryIdException,
BKException.Code.WriteOnReadOnlyBookieException));
private static final int DEFAULT_HIGH_PRIORITY_VALUE = 100; // We may add finer grained priority later.
+ private static final int DEFAULT_CAPACITY = 1_000;
Review Comment:
DEFAULT_CAPACITY > BATCH_RESPONSE_SIZE
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org