You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@bookkeeper.apache.org by "merlimat (via GitHub)" <gi...@apache.org> on 2023/03/04 00:01:29 UTC
[GitHub] [bookkeeper] merlimat opened a new pull request, #3837: Group and flush add-responses after journal sync
merlimat opened a new pull request, #3837:
URL: https://github.com/apache/bookkeeper/pull/3837
### Motivation
Note: this is stacked on top of #3830 & #3835
This change improves the way the AddRequests responses are send to client.
The current flow is:
* The journal-force-thread issues the fsync on the journal file
* We iterate over all the entries that were just synced and for each of them:
1. Trigger channel.writeAndFlus()
2. This will jump on the connection IO thread (Netty will use a `write()` to `eventfd` to post the task and wake the epoll)
3. Write the object in the connection and trigger the serialization logic
4. Grab a `ByteBuf` from the pool and write ~20 bytes with the response
5. Write and flush the buffer on the channel
6. With the flush consolidator we try to group multiple buffer into a single `writev()` syscall, though each call will have a long list of buffer, making the memcpy inefficient.
7. Release all the buffers and return them to the pool
All these steps are quite expensive when the bookie is receiving a lot of small requests.
This PR changes the flow into:
1. journal fsync
2. go through each request and prepare the response into a per-connection `ByteBuf` which is not written on the channel as of yet
3. after preparing all the responses, we flush them at once: Trigger an event on all the connections that will write the accumulated buffers.
The advantages are:
1. 1 ByteBuf allocated per connection instead of 1 per request
1. Less allocations and stress of buffer pool
2. More efficient socket write() operations
3. 1 task per connection posted on the Netty IO threads, instead of 1 per request.
--
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 #3837: Group and flush add-responses after journal sync
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy commented on code in PR #3837:
URL: https://github.com/apache/bookkeeper/pull/3837#discussion_r1129030884
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Journal.java:
##########
@@ -499,6 +502,10 @@ public void run() {
journalStats.getForceWriteGroupingCountStats()
.registerSuccessfulValue(numReqInLastForceWrite);
+ if (requestProcessor != null) {
Review Comment:
Here we trigger all channels to flush `pendingSendResponses`, shall we only pick the requestHandler which is written to the logFile to flush `pendingSendResponses`, it may reduce the unnecessary iteration in the loop.
```
public void flushPendingResponses() {
for (Channel c : allChannels) {
c.pipeline().fireUserEventTriggered(BookieRequestHandler.EVENT_FLUSH_ALL_PENDING_RESPONSES);
}
}
```
If there are 1000 channels, maybe only 2 channels need to flush `pendingSendResponses`
--
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] codecov-commenter commented on pull request #3837: Group and flush add-responses after journal sync
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #3837:
URL: https://github.com/apache/bookkeeper/pull/3837#issuecomment-1457177393
# [Codecov](https://codecov.io/gh/apache/bookkeeper/pull/3837?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#3837](https://codecov.io/gh/apache/bookkeeper/pull/3837?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (162dc8f) into [master](https://codecov.io/gh/apache/bookkeeper/commit/73c5a0e0111754690e5cd74fb030ee64a2f829d9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (73c5a0e) will **decrease** coverage by `22.12%`.
> The diff coverage is `90.38%`.
```diff
@@ Coverage Diff @@
## master #3837 +/- ##
=============================================
- Coverage 54.67% 32.56% -22.12%
+ Complexity 5346 3121 -2225
=============================================
Files 473 473
Lines 40910 40951 +41
Branches 5232 5237 +5
=============================================
- Hits 22366 13334 -9032
- Misses 16462 25964 +9502
+ Partials 2082 1653 -429
```
| Flag | Coverage Δ | |
|---|---|---|
| bookie | `?` | |
| remaining | `29.41% <46.15%> (?)` | |
| replication | `?` | |
| tls | `21.01% <90.38%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/bookkeeper/pull/3837?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...main/java/org/apache/bookkeeper/bookie/Bookie.java](https://codecov.io/gh/apache/bookkeeper/pull/3837?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Ym9va2tlZXBlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvYm9va2llL0Jvb2tpZS5qYXZh) | `75.00% <ø> (ø)` | |
| [...ain/java/org/apache/bookkeeper/bookie/Journal.java](https://codecov.io/gh/apache/bookkeeper/pull/3837?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Ym9va2tlZXBlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvYm9va2llL0pvdXJuYWwuamF2YQ==) | `68.36% <80.00%> (-11.64%)` | :arrow_down: |
| [...pache/bookkeeper/proto/BookieRequestProcessor.java](https://codecov.io/gh/apache/bookkeeper/pull/3837?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Ym9va2tlZXBlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvcHJvdG8vQm9va2llUmVxdWVzdFByb2Nlc3Nvci5qYXZh) | `46.07% <85.71%> (-4.20%)` | :arrow_down: |
| [.../apache/bookkeeper/proto/BookieRequestHandler.java](https://codecov.io/gh/apache/bookkeeper/pull/3837?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Ym9va2tlZXBlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvcHJvdG8vQm9va2llUmVxdWVzdEhhbmRsZXIuamF2YQ==) | `81.81% <87.50%> (+5.81%)` | :arrow_up: |
| [.../java/org/apache/bookkeeper/bookie/BookieImpl.java](https://codecov.io/gh/apache/bookkeeper/pull/3837?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Ym9va2tlZXBlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvYm9va2llL0Jvb2tpZUltcGwuamF2YQ==) | `45.03% <100.00%> (-24.97%)` | :arrow_down: |
| [...g/apache/bookkeeper/proto/BookieProtoEncoding.java](https://codecov.io/gh/apache/bookkeeper/pull/3837?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Ym9va2tlZXBlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvcHJvdG8vQm9va2llUHJvdG9FbmNvZGluZy5qYXZh) | `76.65% <100.00%> (+1.95%)` | :arrow_up: |
| [...java/org/apache/bookkeeper/proto/BookieServer.java](https://codecov.io/gh/apache/bookkeeper/pull/3837?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Ym9va2tlZXBlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvcHJvdG8vQm9va2llU2VydmVyLmphdmE=) | `60.39% <100.00%> (-8.61%)` | :arrow_down: |
| [...g/apache/bookkeeper/proto/WriteEntryProcessor.java](https://codecov.io/gh/apache/bookkeeper/pull/3837?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Ym9va2tlZXBlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvcHJvdG8vV3JpdGVFbnRyeVByb2Nlc3Nvci5qYXZh) | `41.42% <100.00%> (-28.02%)` | :arrow_down: |
| [...java/org/apache/bookkeeper/proto/BookieClient.java](https://codecov.io/gh/apache/bookkeeper/pull/3837?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Ym9va2tlZXBlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvcHJvdG8vQm9va2llQ2xpZW50LmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...org/apache/bookkeeper/bookie/ReadOnlyFileInfo.java](https://codecov.io/gh/apache/bookkeeper/pull/3837?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Ym9va2tlZXBlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvYm9va2llL1JlYWRPbmx5RmlsZUluZm8uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [340 more](https://codecov.io/gh/apache/bookkeeper/pull/3837?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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 #3837: Group and flush add-responses after journal sync
Posted by "zymap (via GitHub)" <gi...@apache.org>.
zymap commented on code in PR #3837:
URL: https://github.com/apache/bookkeeper/pull/3837#discussion_r1127213633
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Journal.java:
##########
@@ -1093,6 +1100,10 @@ journalFormatVersionToWrite, getBufferedChannelBuilder(),
numEntriesToFlush--;
entry.run();
}
+
+ if (forceWriteThread.requestProcessor != null) {
+ forceWriteThread.requestProcessor.flushPendingResponses();
+ }
Review Comment:
Do we need to record [`journalStats.getJournalFlushStats()`](https://github.com/apache/bookkeeper/pull/3837/files#diff-0a79d54e3dfd4bab1d85510ef957f8f4b7ab6f6acd09aadd634b4d7bbee6e1f4R1111) before trigger flushPendingResponses? That would lead the `JournalFlushStats` to include response time.
--
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] merlimat commented on a diff in pull request #3837: Group and flush add-responses after journal sync
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat commented on code in PR #3837:
URL: https://github.com/apache/bookkeeper/pull/3837#discussion_r1127263449
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Journal.java:
##########
@@ -1093,6 +1100,10 @@ journalFormatVersionToWrite, getBufferedChannelBuilder(),
numEntriesToFlush--;
entry.run();
}
+
+ if (forceWriteThread.requestProcessor != null) {
+ forceWriteThread.requestProcessor.flushPendingResponses();
+ }
Review Comment:
The flush would be very fast though. It's only posting an event on the channel group, not writing the actuals responses on the channels.
--
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 merged pull request #3837: Group and flush add-responses after journal sync
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 merged PR #3837:
URL: https://github.com/apache/bookkeeper/pull/3837
--
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