Skip to content

Remove callback threadpool for sending add responses#3825

Merged
zymap merged 2 commits intoapache:masterfrom
merlimat:add-response-callbacks
Mar 1, 2023
Merged

Remove callback threadpool for sending add responses#3825
zymap merged 2 commits intoapache:masterfrom
merlimat:add-response-callbacks

Conversation

@merlimat
Copy link
Copy Markdown
Contributor

@merlimat merlimat commented Mar 1, 2023

Motivation

The option of triggering journal callbacks from a thread-pool instead of from the JournalForceWrite thread is not really helpful under any condition.

The only thing this option does is to introduce an extra context switch to jump on this thread-pool and from there it will trigger the response, though basically no work is done in this thread-pool. It will just jump on the Netty IO-thread for the specific connection and from there the serialization and the socket write will be done.

When using the thread-pool, one can see the effect of the extra-context switch and the contention on the execution.

Additionally, when 0 threads are configured, we are using the Guava DirectExecutor which has non-zero overhead in the form of a mutex that is contended between multiple journal threads.

@merlimat merlimat added this to the 4.16.0 milestone Mar 1, 2023
@merlimat merlimat requested review from hangc0276 and zymap March 1, 2023 01:37
@merlimat merlimat self-assigned this Mar 1, 2023
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

Merging #3825 (8dba661) into master (6d00336) will decrease coverage by 10.10%.
The diff coverage is 100.00%.

@@              Coverage Diff              @@
##             master    #3825       +/-   ##
=============================================
- Coverage     68.37%   58.28%   -10.10%     
+ Complexity     6759     5687     -1072     
=============================================
  Files           473      473               
  Lines         40972    40963        -9     
  Branches       5241     5239        -2     
=============================================
- Hits          28015    23875     -4140     
- Misses        10699    14983     +4284     
+ Partials       2258     2105      -153     
Flag Coverage Δ
bookie ?
client 44.17% <100.00%> (-0.01%) ⬇️
remaining 29.59% <66.66%> (+0.08%) ⬆️
replication 41.39% <66.66%> (-0.05%) ⬇️
tls 20.96% <66.66%> (-0.09%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...rg/apache/bookkeeper/conf/ServerConfiguration.java 63.45% <ø> (-14.22%) ⬇️
...ain/java/org/apache/bookkeeper/bookie/Journal.java 76.25% <100.00%> (-6.12%) ⬇️
...org/apache/bookkeeper/bookie/ReadOnlyFileInfo.java 0.00% <0.00%> (-100.00%) ⬇️
...org/apache/bookkeeper/bookie/datainteg/Events.java 0.00% <0.00%> (-100.00%) ⬇️
.../apache/bookkeeper/bookie/storage/EntryLogger.java 0.00% <0.00%> (-100.00%) ⬇️
...eeper/bookie/storage/directentrylogger/Events.java 0.00% <0.00%> (-100.00%) ⬇️
...e/bookkeeper/bookie/datainteg/EntryCopierImpl.java 0.00% <0.00%> (-97.50%) ⬇️
.../apache/bookkeeper/bookie/datainteg/WriteSets.java 0.00% <0.00%> (-96.43%) ⬇️
.../storage/directentrylogger/WriterWithMetadata.java 0.00% <0.00%> (-95.66%) ⬇️
...ookie/storage/directentrylogger/LogReaderScan.java 0.00% <0.00%> (-95.24%) ⬇️
... and 122 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@zymap zymap merged commit 3dcd8d5 into apache:master Mar 1, 2023
Ghatage pushed a commit to sijie/bookkeeper that referenced this pull request Jul 12, 2024
### Motivation

The option of triggering journal callbacks from a thread-pool instead of from the JournalForceWrite thread is not really helpful under any condition.

The only thing this option does is to introduce an extra context switch to jump on this thread-pool and from there it will trigger the response, though basically no work is done in this thread-pool. It will just jump on the Netty IO-thread for the specific connection and from there the serialization and the socket write will be done.

When using the thread-pool, one can see the effect of the extra-context switch and the contention on the execution.

Additionally, when 0 threads are configured, we are using the Guava DirectExecutor which has non-zero overhead in the form of a mutex that is contended between multiple journal threads.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants