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.

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

[fix][broker] Fix cursor should use latest ledger config #22644

Merged
merged 3 commits into from
May 10, 2024

Conversation

nodece
Copy link
Member

@nodece nodece commented May 3, 2024

Motivation

When the ledger opens a cursor, the cursor uses a field to hold a managed ledger configuration. Once the managed ledger configuration is renewed(The managed ledger configuration will be renewed when the namespace/topic policies change) in the managed ledger, the cursor cannot use the latest ledger config.

This will break the ledger feature like autoSkipNonRecoverableData.

Modifications

  • Remove the config parameter in the constructor of ManagedCursorImpl and NonDurableCursorImpl
  • Use cursor.getConfig instead of cursor.config, which method always gets the managed ledger config for managed ldeger.

Documentation

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

Signed-off-by: Zixuan Liu <nodeces@gmail.com>
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label May 3, 2024
Copy link
Member

@dao-jun dao-jun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@lhotari
Copy link
Member

lhotari commented May 3, 2024

a test needs updating:

BrokerBkEnsemblesTests.testSkipCorruptDataLedger:213 » NoSuchField config

@dao-jun
Copy link
Member

dao-jun commented May 3, 2024

This lead to too many changes, can we just update Cursor#config when the config changed?

@nodece
Copy link
Member Author

nodece commented May 4, 2024

This lead to too many changes, can we just update Cursor#config when the config changed?

Sounds like a good idea, but we need to add a new method to cursor interface.

@dao-jun
Copy link
Member

dao-jun commented May 4, 2024

This lead to too many changes, can we just update Cursor#config when the config changed?

Sounds like a good idea, but we need to add a new method to cursor interface.

Yes, but I believe it's a better way

@nodece
Copy link
Member Author

nodece commented May 4, 2024

@lhotari Do you have any suggestions?

nodece added 2 commits May 5, 2024 00:38
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 87.50000% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 72.68%. Comparing base (bbc6224) to head (ab02f3a).
Report is 224 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #22644      +/-   ##
============================================
- Coverage     73.57%   72.68%   -0.89%     
+ Complexity    32624    32324     -300     
============================================
  Files          1877     1887      +10     
  Lines        139502   141018    +1516     
  Branches      15299    15480     +181     
============================================
- Hits         102638   102504     -134     
- Misses        28908    30648    +1740     
+ Partials       7956     7866      -90     
Flag Coverage Δ
inttests 27.32% <52.50%> (+2.74%) ⬆️
systests 24.58% <35.00%> (+0.25%) ⬆️
unittests 71.46% <87.50%> (-1.39%) ⬇️

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

Files Coverage Δ
...okkeeper/mledger/impl/ManagedCursorMXBeanImpl.java 93.93% <100.00%> (+0.18%) ⬆️
...che/bookkeeper/mledger/impl/ManagedLedgerImpl.java 80.92% <100.00%> (+0.26%) ⬆️
.../bookkeeper/mledger/impl/NonDurableCursorImpl.java 81.25% <100.00%> (ø)
...pache/bookkeeper/mledger/impl/RangeSetWrapper.java 94.33% <100.00%> (ø)
...he/bookkeeper/mledger/impl/ReadOnlyCursorImpl.java 95.00% <100.00%> (ø)
...rg/apache/bookkeeper/mledger/impl/OpReadEntry.java 84.31% <0.00%> (ø)
...keeper/mledger/impl/ReadOnlyManagedLedgerImpl.java 56.16% <0.00%> (+2.54%) ⬆️
...che/bookkeeper/mledger/impl/ManagedCursorImpl.java 80.03% <89.65%> (+0.73%) ⬆️

... and 342 files with indirect coverage changes

@nodece nodece self-assigned this May 6, 2024
@nodece nodece requested review from lhotari and poorbarcode May 6, 2024 14:57
@dao-jun dao-jun added this to the 3.4.0 milestone May 9, 2024
@nodece nodece merged commit 774a5d4 into apache:master May 10, 2024
50 of 52 checks passed
nodece added a commit to nodece/pulsar that referenced this pull request May 11, 2024
Signed-off-by: Zixuan Liu <nodeces@gmail.com>

(cherry picked from commit 774a5d4)
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
nodece added a commit to nodece/pulsar that referenced this pull request May 11, 2024
Signed-off-by: Zixuan Liu <nodeces@gmail.com>

(cherry picked from commit 774a5d4)
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
nodece added a commit to nodece/pulsar that referenced this pull request May 11, 2024
Signed-off-by: Zixuan Liu <nodeces@gmail.com>

(cherry picked from commit 774a5d4)
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
nodece added a commit to nodece/pulsar that referenced this pull request May 11, 2024
Signed-off-by: Zixuan Liu <nodeces@gmail.com>

(cherry picked from commit 774a5d4)
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
nodece added a commit to nodece/pulsar that referenced this pull request May 11, 2024
Signed-off-by: Zixuan Liu <nodeces@gmail.com>

(cherry picked from commit 774a5d4)
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
@nodece nodece deleted the fix-cursor-use-latest-ledger-config branch May 11, 2024 14:54
nodece added a commit to ascentstream/pulsar that referenced this pull request May 11, 2024
… (#13)

(cherry picked from commit 774a5d4)

Signed-off-by: Zixuan Liu <nodeces@gmail.com>
Technoboy- pushed a commit that referenced this pull request May 15, 2024
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
lhotari pushed a commit that referenced this pull request Jun 4, 2024
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
(cherry picked from commit 774a5d4)
coderzc pushed a commit that referenced this pull request Jun 5, 2024
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
(cherry picked from commit 774a5d4)
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 7, 2024
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
(cherry picked from commit 774a5d4)
(cherry picked from commit 919cfeb)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 7, 2024
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
(cherry picked from commit 774a5d4)
(cherry picked from commit 919cfeb)
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.

6 participants