Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[improve][broker] Pin executor to managed ledger instance to cache the string hashing #18078

Merged
merged 8 commits into from
Nov 30, 2022
Merged

[improve][broker] Pin executor to managed ledger instance to cache the string hashing #18078

merged 8 commits into from
Nov 30, 2022

Conversation

merlimat
Copy link
Contributor

@merlimat merlimat commented Oct 17, 2022 •
edited
Loading

Motivation

In many operations we're hashing on the managed ledger name to ensure all the callbacks are triggered from same thread. Instead of hashing the string each, let's pin the thread and always use the same, for a given managed ledger instance.

This will avoid the hashing overhead.

Tests in merlimat#3

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@merlimat merlimat added the type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages label Oct 17, 2022
@merlimat merlimat added this to the 2.12.0 milestone Oct 17, 2022
@merlimat merlimat added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Oct 17, 2022
@github-actions github-actions bot added doc-label-missing and removed doc-not-needed Your PR changes do not impact docs labels Oct 17, 2022
@apache apache deleted a comment from github-actions bot Oct 17, 2022
@merlimat merlimat added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Oct 17, 2022
@github-actions github-actions bot added doc-label-missing and removed doc-not-needed Your PR changes do not impact docs labels Oct 17, 2022
@github-actions
Copy link

@merlimat Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

@github-actions github-actions bot added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Oct 17, 2022
@merlimat merlimat changed the title [Broker] Pin executor to managed ledger instance to cache the string hashing [improve] Pin executor to managed ledger instance to cache the string hashing Oct 17, 2022
@Technoboy- Technoboy- changed the title [improve] Pin executor to managed ledger instance to cache the string hashing [improve][broker] Pin executor to managed ledger instance to cache the string hashing Oct 18, 2022
@Technoboy- Technoboy- closed this Oct 18, 2022
@Technoboy- Technoboy- reopened this Oct 18, 2022
@Technoboy- Technoboy- mentioned this pull request Oct 18, 2022
Merged
4 tasks
@codecov-commenter
Copy link

codecov-commenter commented Oct 18, 2022 •
edited
Loading

Codecov Report

Merging #18078 (66fc80a) into master (cee697b) will decrease coverage by 0.49%.
The diff coverage is 19.42%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #18078      +/-   ##
============================================
- Coverage     47.06%   46.56%   -0.50%     
- Complexity     8956    10342    +1386     
============================================
  Files           592      698     +106     
  Lines         56313    68326   +12013     
  Branches       5844     7332    +1488     
============================================
+ Hits          26503    31817    +5314     
- Misses        26946    32886    +5940     
- Partials       2864     3623     +759
Flag Coverage Δ
unittests 46.56% <19.42%> (-0.50%) ⬇️

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

Impacted Files Coverage Δ
...apache/bookkeeper/mledger/ManagedLedgerConfig.java 71.60% <0.00%> (ø)
.../org/apache/bookkeeper/mledger/impl/MetaStore.java 100.00% <ø> (ø)
...okkeeper/mledger/impl/ShadowManagedLedgerImpl.java 0.00% <0.00%> (ø)
...kkeeper/mledger/impl/cache/EntryCacheDisabled.java 10.86% <0.00%> (ø)
...ava/org/apache/pulsar/broker/service/Consumer.java 70.28% <0.00%> (+8.18%) ⬆️
...ker/stats/prometheus/NamespaceStatsAggregator.java 0.00% <0.00%> (ø)
...va/org/apache/pulsar/client/impl/ProducerImpl.java 17.00% <0.00%> (-0.02%) ⬇️
...rg/apache/pulsar/broker/service/BrokerService.java 57.50% <20.00%> (+0.31%) ⬆️
...sar/broker/service/persistent/PersistentTopic.java 61.12% <20.00%> (-1.16%) ⬇️
...ava/org/apache/pulsar/broker/service/Producer.java 61.75% <27.27%> (-2.15%) ⬇️
... and 191 more

@Technoboy- Technoboy- force-pushed the managed-ledger-executor-cache branch from c27ba86 to a971e4c Compare October 22, 2022 14:16
@poorbarcode
Copy link
Contributor

/pulsarbot rerun-failure-checks

@tisonkun
Copy link
Member

tisonkun commented Nov 2, 2022

/pulsarbot rerun-failure-checks

@Technoboy- Technoboy- force-pushed the managed-ledger-executor-cache branch from a971e4c to 46f1b28 Compare November 5, 2022 01:06
@Technoboy- Technoboy- closed this Nov 19, 2022
@Technoboy- Technoboy- reopened this Nov 19, 2022
@Technoboy- Technoboy- force-pushed the managed-ledger-executor-cache branch from 46f1b28 to 7082989 Compare November 19, 2022 05:13
@AnonHxy AnonHxy mentioned this pull request Nov 22, 2022
Merged
5 tasks
@Technoboy- Technoboy- force-pushed the managed-ledger-executor-cache branch from 7082989 to 814a24c Compare November 23, 2022 01:50
@AnonHxy AnonHxy force-pushed the managed-ledger-executor-cache branch from 8a7a7ce to 9783b6b Compare November 28, 2022 16:24
@AnonHxy AnonHxy force-pushed the managed-ledger-executor-cache branch from 9783b6b to 66fc80a Compare November 29, 2022 05:05
@AnonHxy AnonHxy merged commit fe45a9c into apache:master Nov 30, 2022
lifepuzzlefun pushed a commit to lifepuzzlefun/pulsar that referenced this pull request Dec 9, 2022
@merlimat @Technoboy-
@AnonHxy @lifepuzzlefun
[improve][broker] Pin executor to managed ledger instance to cache th…
7485127 
…e string hashing (apache#18078)

Co-authored-by: Jiwe Guo <[email protected]>
Co-authored-by: houxiaoyu <[email protected]>
lifepuzzlefun pushed a commit to lifepuzzlefun/pulsar that referenced this pull request Jan 10, 2023
@merlimat @Technoboy-
@AnonHxy @lifepuzzlefun
[improve][broker] Pin executor to managed ledger instance to cache th…
2e01dfa 
…e string hashing (apache#18078)

Co-authored-by: Jiwe Guo <[email protected]>
Co-authored-by: houxiaoyu <[email protected]>
@AnonHxy AnonHxy mentioned this pull request Jan 17, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaeljmarshall michaeljmarshall michaeljmarshall approved these changes

@coderzc coderzc coderzc approved these changes

@tisonkun tisonkun tisonkun approved these changes

@lifepuzzlefun lifepuzzlefun lifepuzzlefun approved these changes

@codelipenghui codelipenghui codelipenghui approved these changes

@AnonHxy AnonHxy AnonHxy approved these changes

@Technoboy- Technoboy- Technoboy- approved these changes

@lhotari lhotari Awaiting requested review from lhotari

@hangc0276 hangc0276 Awaiting requested review from hangc0276

@eolivelli eolivelli Awaiting requested review from eolivelli

Assignees

@merlimat merlimat

Labels
doc-not-needed Your PR changes do not impact docs ready-to-test type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.
10 participants
@merlimat @codecov-commenter @poorbarcode @tisonkun @Technoboy- @AnonHxy @codelipenghui @lifepuzzlefun @coderzc @michaeljmarshall