You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@bookkeeper.apache.org by GitBox <gi...@apache.org> on 2023/01/16 05:24:45 UTC
[GitHub] [bookkeeper] AnonHxy opened a new pull request, #3738: Pin order executor in LedgerHandle
AnonHxy opened a new pull request, #3738:
URL: https://github.com/apache/bookkeeper/pull/3738
Descriptions of the changes in this PR:
### Motivation
Pin the order executor in `LedgerHandle` to save the choose thread overhead
### Changes
Choose the order executor in `LedgerHandle` constructor method and save it as a final field
--
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] eolivelli merged pull request #3738: Pin order executor in LedgerHandle
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli merged PR #3738:
URL: https://github.com/apache/bookkeeper/pull/3738
--
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] AnonHxy commented on a diff in pull request #3738: Pin order executor in LedgerHandle
Posted by "AnonHxy (via GitHub)" <gi...@apache.org>.
AnonHxy commented on code in PR #3738:
URL: https://github.com/apache/bookkeeper/pull/3738#discussion_r1089874399
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/MockBookKeeper.java:
##########
@@ -49,7 +48,10 @@
*/
public class MockBookKeeper extends BookKeeper {
- final ExecutorService executor = Executors.newFixedThreadPool(1, new DefaultThreadFactory("mock-bookkeeper"));
+ final OrderedExecutor orderedExecutor = OrderedExecutor.newBuilder()
Review Comment:
@hangc0276 We need call `getMainWorkerPool()` in `LedgerHandle` constructor method. If we didn't change it, the `getMainWorkerPool()` will return null and throw NPE in the constructor method:
[Line199](https://github.com/apache/bookkeeper/pull/3738/files#diff-1d893bb31553b5e1f55c8301d04ae15f38e0d35f531f9dd22475128b7972ddf9R197-R199)
--
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] AnonHxy commented on pull request #3738: Pin order executor in LedgerHandle
Posted by "AnonHxy (via GitHub)" <gi...@apache.org>.
AnonHxy commented on PR #3738:
URL: https://github.com/apache/bookkeeper/pull/3738#issuecomment-1407421059
> when the execution mode changes, the performance change significantly ? ledgerID will block reading. Is it expected?
I think this patch will not change the executeion mode. It just make the execute thread as a final field, which is a obvious improvement. But I am not sure how much can be improved. @StevenLuMT
--
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] AnonHxy commented on pull request #3738: Pin order executor in LedgerHandle
Posted by "AnonHxy (via GitHub)" <gi...@apache.org>.
AnonHxy commented on PR #3738:
URL: https://github.com/apache/bookkeeper/pull/3738#issuecomment-1407572444
> Overall LGTM. This change saves the cost of choosing threads in executeOrdered, and I wonder if choosing threads costs a lot of CPU resources and whether this change can improve the performance or not.
I tested it using one bookie client to add entry async like following. It looks that it was not a significant improvement from the result.
```
for (int i = 0; i < Integer.MAX_VALUE; ++i) {
handle.asyncAddEntry("Some entry data".getBytes(), new AsyncCallback.AddCallback() {
@Override
public void addComplete(int rc, LedgerHandle lh, long entryId, Object ctx) {
}
}, null);
}
```
--
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 pull request #3738: Pin order executor in LedgerHandle
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on PR #3738:
URL: https://github.com/apache/bookkeeper/pull/3738#issuecomment-1384732952
@AnonHxy Would you please provide more context about the reason to pin the executor? thanks.
--
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 #3738: Pin order executor in LedgerHandle
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 commented on code in PR #3738:
URL: https://github.com/apache/bookkeeper/pull/3738#discussion_r1089847473
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/MockBookKeeper.java:
##########
@@ -49,7 +48,10 @@
*/
public class MockBookKeeper extends BookKeeper {
- final ExecutorService executor = Executors.newFixedThreadPool(1, new DefaultThreadFactory("mock-bookkeeper"));
+ final OrderedExecutor orderedExecutor = OrderedExecutor.newBuilder()
Review Comment:
Why change this thread pool?
--
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] AnonHxy commented on a diff in pull request #3738: Pin order executor in LedgerHandle
Posted by "AnonHxy (via GitHub)" <gi...@apache.org>.
AnonHxy commented on code in PR #3738:
URL: https://github.com/apache/bookkeeper/pull/3738#discussion_r1089874399
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/MockBookKeeper.java:
##########
@@ -49,7 +48,10 @@
*/
public class MockBookKeeper extends BookKeeper {
- final ExecutorService executor = Executors.newFixedThreadPool(1, new DefaultThreadFactory("mock-bookkeeper"));
+ final OrderedExecutor orderedExecutor = OrderedExecutor.newBuilder()
Review Comment:
We need call `getMainWorkerPool()` in `LedgerHandle` constructor method. If we didn't change it, the `getMainWorkerPool()` will return null and throw NPE in the constructor method:
[Line199](https://github.com/apache/bookkeeper/pull/3738/files#diff-1d893bb31553b5e1f55c8301d04ae15f38e0d35f531f9dd22475128b7972ddf9R197-R199)
--
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] AnonHxy commented on pull request #3738: Pin order executor in LedgerHandle
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #3738:
URL: https://github.com/apache/bookkeeper/pull/3738#issuecomment-1383925809
PTAL @StevenLuMT @eolivelli
--
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] AnonHxy commented on pull request #3738: Pin order executor in LedgerHandle
Posted by "AnonHxy (via GitHub)" <gi...@apache.org>.
AnonHxy commented on PR #3738:
URL: https://github.com/apache/bookkeeper/pull/3738#issuecomment-1399197933
PTAL @hangc0276 @eolivelli , thanks
--
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] AnonHxy commented on pull request #3738: Pin order executor in LedgerHandle
Posted by "AnonHxy (via GitHub)" <gi...@apache.org>.
AnonHxy commented on PR #3738:
URL: https://github.com/apache/bookkeeper/pull/3738#issuecomment-1633572323
@eolivelli @dlg99 @merlimat PTAL, thanks:)
--
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] AnonHxy commented on pull request #3738: Pin order executor in LedgerHandle
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #3738:
URL: https://github.com/apache/bookkeeper/pull/3738#issuecomment-1384747466
> @AnonHxy Would you please provide more context about the reason to pin the executor? thanks.
Sure. It was inspired by this PR https://github.com/apache/pulsar/pull/18078 @hangc0276
--
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] AnonHxy commented on pull request #3738: Pin order executor in LedgerHandle
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #3738:
URL: https://github.com/apache/bookkeeper/pull/3738#issuecomment-1383610135
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] hangc0276 commented on pull request #3738: Pin order executor in LedgerHandle
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 commented on PR #3738:
URL: https://github.com/apache/bookkeeper/pull/3738#issuecomment-1409583667
> > Overall LGTM. This change saves the cost of choosing threads in executeOrdered, and I wonder if choosing threads costs a lot of CPU resources and whether this change can improve the performance or not.
>
> I tested it using one bookie client to add entry async like following. It looks that it was not a significant improvement from the result.
>
> ```
> for (int i = 0; i < Integer.MAX_VALUE; ++i) {
> handle.asyncAddEntry("Some entry data".getBytes(), new AsyncCallback.AddCallback() {
> @Override
> public void addComplete(int rc, LedgerHandle lh, long entryId, Object ctx) {
>
> }
> }, null);
> }
> ```
@AnonHxy If there is no significant performance improvement, I think we can hold this change.
--
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 pull request #3738: Pin order executor in LedgerHandle
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 commented on PR #3738:
URL: https://github.com/apache/bookkeeper/pull/3738#issuecomment-1534078309
@AnonHxy Do you have any updates?
--
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] AnonHxy commented on pull request #3738: Pin order executor in LedgerHandle
Posted by "AnonHxy (via GitHub)" <gi...@apache.org>.
AnonHxy commented on PR #3738:
URL: https://github.com/apache/bookkeeper/pull/3738#issuecomment-1534142530
>
> I think we can hold on this change.
OK. But I think this change is a positive optimization at least, while the proform result is not obvious ... @hangc0276
--
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