You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@bookkeeper.apache.org by "horizonzy (via GitHub)" <gi...@apache.org> on 2023/03/09 13:15:19 UTC
[GitHub] [bookkeeper] horizonzy opened a new pull request, #3848: Improve group and flush add-responses after journal sync
horizonzy opened a new pull request, #3848:
URL: https://github.com/apache/bookkeeper/pull/3848
Descriptions of the changes in this PR:
This is an improvement for #3837
### Motivation
1. Now if the maxPendingResponsesSize is expanded large, it will not decrease. => We should make it flexible.
2. Now after prepareSendResponseV2 to the channel, then we trigger all channels to flush pendingSendResponses, maybe there is only a few channels that need to flush, but if we trigger all channels, it's a waste. => We only flush the channel which prepareSendResponseV2.
--
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 closed pull request #3848: Improve group and flush add-responses after journal sync
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy closed pull request #3848: Improve group and flush add-responses after journal sync
URL: https://github.com/apache/bookkeeper/pull/3848
--
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 pull request #3848: Improve group and flush add-responses after journal sync
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy commented on PR #3848:
URL: https://github.com/apache/bookkeeper/pull/3848#issuecomment-1461146908
rerun failure checks
--
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 pull request #3848: Improve group and flush add-responses after journal sync
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy commented on PR #3848:
URL: https://github.com/apache/bookkeeper/pull/3848#issuecomment-1466405778
rerun failure checks
--
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 #3848: Improve group and flush add-responses after journal sync
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat commented on code in PR #3848:
URL: https://github.com/apache/bookkeeper/pull/3848#discussion_r1131230075
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Journal.java:
##########
@@ -503,17 +513,18 @@ public void run() {
// responses
for (int i = 0; i < requestsCount; i++) {
ForceWriteRequest req = localRequests.get(i);
- numReqInLastForceWrite += req.process();
+ numReqInLastForceWrite += req.process(writeHandlers);
req.recycle();
}
journalStats.getForceWriteGroupingCountStats()
.registerSuccessfulValue(numReqInLastForceWrite);
-
- if (requestProcessor != null) {
- requestProcessor.flushPendingResponses();
+ if (writeHandlers.size() > 0) {
+ for (ObjectCursor<BookieRequestHandler> writeHandler : writeHandlers) {
+ writeHandler.value.flushPendingResponse();
+ }
+ writeHandlers.clear();
}
Review Comment:
In this case it would efficient to use the lambda approach because it would be non-capturing and `ObjectHashSet` is not going to create an iterator either.
```suggestion
writeHandlers.forEach(wh -> wh.flushPendingResponse());
writeHandlers.clear();
```
--
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 pull request #3848: Improve group and flush add-responses after journal sync
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy commented on PR #3848:
URL: https://github.com/apache/bookkeeper/pull/3848#issuecomment-1463362675
rerun failure checks
--
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 #3848: Improve group and flush add-responses after journal sync
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #3848:
URL: https://github.com/apache/bookkeeper/pull/3848#issuecomment-1460373971
# [Codecov](https://codecov.io/gh/apache/bookkeeper/pull/3848?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 [#3848](https://codecov.io/gh/apache/bookkeeper/pull/3848?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b46fe9c) into [master](https://codecov.io/gh/apache/bookkeeper/commit/f9313195f017ce47f3cb1900d4db6ad8b261ad7e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f931319) will **decrease** coverage by `38.79%`.
> The diff coverage is `38.46%`.
```diff
@@ Coverage Diff @@
## master #3848 +/- ##
=============================================
- Coverage 68.36% 29.58% -38.79%
+ Complexity 6780 2881 -3899
=============================================
Files 473 473
Lines 40982 40979 -3
Branches 5241 5241
=============================================
- Hits 28019 12125 -15894
- Misses 10713 27280 +16567
+ Partials 2250 1574 -676
```
| Flag | Coverage Δ | |
|---|---|---|
| bookie | `?` | |
| client | `?` | |
| remaining | `29.58% <38.46%> (+0.16%)` | :arrow_up: |
| replication | `?` | |
| tls | `?` | |
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/3848?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/3848?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% <ø> (ø)` | |
| [.../java/org/apache/bookkeeper/bookie/BookieImpl.java](https://codecov.io/gh/apache/bookkeeper/pull/3848?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==) | `44.64% <ø> (-26.82%)` | :arrow_down: |
| [...pache/bookkeeper/proto/BookieRequestProcessor.java](https://codecov.io/gh/apache/bookkeeper/pull/3848?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) | `26.45% <ø> (-48.16%)` | :arrow_down: |
| [...java/org/apache/bookkeeper/proto/BookieServer.java](https://codecov.io/gh/apache/bookkeeper/pull/3848?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=) | `56.00% <ø> (-15.29%)` | :arrow_down: |
| [.../apache/bookkeeper/proto/BookieRequestHandler.java](https://codecov.io/gh/apache/bookkeeper/pull/3848?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==) | `46.15% <20.00%> (-40.21%)` | :arrow_down: |
| [...ain/java/org/apache/bookkeeper/bookie/Journal.java](https://codecov.io/gh/apache/bookkeeper/pull/3848?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==) | `67.66% <50.00%> (-12.52%)` | :arrow_down: |
| [...ain/java/org/apache/bookkeeper/auth/AuthToken.java](https://codecov.io/gh/apache/bookkeeper/pull/3848?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Ym9va2tlZXBlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvYXV0aC9BdXRoVG9rZW4uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/bookkeeper/proto/BookieClient.java](https://codecov.io/gh/apache/bookkeeper/pull/3848?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: |
| [...a/org/apache/bookkeeper/client/api/BookKeeper.java](https://codecov.io/gh/apache/bookkeeper/pull/3848?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Ym9va2tlZXBlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvY2xpZW50L2FwaS9Cb29rS2VlcGVyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...a/org/apache/bookkeeper/client/api/ReadHandle.java](https://codecov.io/gh/apache/bookkeeper/pull/3848?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Ym9va2tlZXBlci1zZXJ2ZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvY2xpZW50L2FwaS9SZWFkSGFuZGxlLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [364 more](https://codecov.io/gh/apache/bookkeeper/pull/3848?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] merlimat commented on a diff in pull request #3848: Improve group and flush add-responses after journal sync
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat commented on code in PR #3848:
URL: https://github.com/apache/bookkeeper/pull/3848#discussion_r1131225156
##########
bookkeeper-server/pom.xml:
##########
@@ -149,6 +149,10 @@
<scope>runtime</scope>
<optional>true</optional>
</dependency>
+ <dependency>
+ <groupId>com.carrotsearch</groupId>
+ <artifactId>hppc</artifactId>
Review Comment:
We should also add to license file
--
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 closed pull request #3848: Improve group and flush add-responses after journal sync
Posted by "horizonzy (via GitHub)" <gi...@apache.org>.
horizonzy closed pull request #3848: Improve group and flush add-responses after journal sync
URL: https://github.com/apache/bookkeeper/pull/3848
--
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 #3848: Improve group and flush add-responses after journal sync
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat commented on code in PR #3848:
URL: https://github.com/apache/bookkeeper/pull/3848#discussion_r1130564777
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Journal.java:
##########
@@ -503,17 +513,26 @@ public void run() {
// responses
for (int i = 0; i < requestsCount; i++) {
ForceWriteRequest req = localRequests.get(i);
+ req.getForceWriteWaiters().forEach(ele -> {
Review Comment:
`req.process()` is already iterating over all the entries for each `ForceWriteRequest`. It would be better to reuse the same loop.
Additionally using `forEach` has 2 problems:
1. allocates an iterator, unlike doing `for (int i =0; i... .)`
2. the lambda is also capturing variables and thus it will allocate a temporary object
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Journal.java:
##########
@@ -495,6 +504,7 @@ public void run() {
journalStats.getForceWriteQueueSize().addCount(-requestsCount);
+ Set<BookieRequestHandler> writeHandlers = new HashSet<>();
Review Comment:
Using the set has its own overhead too. In this case it will allocate a `Node` object for the linked list for each entry added here.
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Journal.java:
##########
@@ -495,6 +504,7 @@ public void run() {
journalStats.getForceWriteQueueSize().addCount(-requestsCount);
+ Set<BookieRequestHandler> writeHandlers = new HashSet<>();
Review Comment:
Perhaps we could use `ObjectHashSet` from carrotsearch https://github.com/carrotsearch/hppc
It's already used in Pulsar too.
We should also reuse the same set each time.
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieRequestHandler.java:
##########
@@ -92,31 +92,22 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception
public synchronized void prepareSendResponseV2(int rc, BookieProtocol.ParsedAddRequest req) {
if (pendingSendResponses == null) {
- pendingSendResponses = ctx.alloc().directBuffer(maxPendingResponsesSize != 0
- ? maxPendingResponsesSize : 256);
+ pendingSendResponses = ctx().alloc().directBuffer(maxPendingResponsesSize);
}
-
BookieProtoEncoding.ResponseEnDeCoderPreV3.serializeAddResponseInto(rc, req, pendingSendResponses);
}
- @Override
- public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exception {
- if (evt == EVENT_FLUSH_ALL_PENDING_RESPONSES) {
- synchronized (this) {
- if (pendingSendResponses != null) {
- maxPendingResponsesSize = Math.max(maxPendingResponsesSize,
- pendingSendResponses.readableBytes());
- if (ctx.channel().isActive()) {
- ctx.writeAndFlush(pendingSendResponses, ctx.voidPromise());
- } else {
- pendingSendResponses.release();
- }
-
- pendingSendResponses = null;
- }
+ public synchronized void flushPendingResponse() {
+ if (pendingSendResponses != null) {
+ maxPendingResponsesSize = (int) Math.max(
+ maxPendingResponsesSize * 0.9 + 0.1 * pendingSendResponses.readableBytes(),
Review Comment:
This looks like it's going to take quite a bit of iterations to expand the cluster size. While in the meantime we have to keep expanding the `ByteBuf`, which will incur in multiple allocations and memory copies
--
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 #3848: Improve group and flush add-responses after journal sync
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 merged PR #3848:
URL: https://github.com/apache/bookkeeper/pull/3848
--
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] wenbingshen commented on pull request #3848: Improve group and flush add-responses after journal sync
Posted by "wenbingshen (via GitHub)" <gi...@apache.org>.
wenbingshen commented on PR #3848:
URL: https://github.com/apache/bookkeeper/pull/3848#issuecomment-1469907665
rerun failure checks
--
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