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] [ml] Fix cursor metadata compatability issue when switching the config unackedRangesOpenCacheSetEnabled #23759

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

poorbarcode
Copy link
Contributor

@poorbarcode poorbarcode commented Dec 19, 2024

Motivation

Modifications

Documentation

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

Matching PR in forked repository

PR in forked repository: x

@poorbarcode poorbarcode added type/bug The PR fixed a bug or issue reported a bug release/4.0.2 labels Dec 19, 2024
@poorbarcode poorbarcode self-assigned this Dec 19, 2024
@poorbarcode poorbarcode added this to the 4.1.0 milestone Dec 19, 2024
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Dec 19, 2024
@poorbarcode
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

@codecov-commenter
Copy link

codecov-commenter commented Dec 19, 2024

Codecov Report

Attention: Patch coverage is 56.66667% with 13 lines in your changes missing coverage. Please review.

Project coverage is 74.13%. Comparing base (bbc6224) to head (f2a2441).
Report is 815 commits behind head on master.

Files with missing lines Patch % Lines
...che/bookkeeper/mledger/impl/ManagedCursorImpl.java 31.57% 13 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #23759      +/-   ##
============================================
+ Coverage     73.57%   74.13%   +0.56%     
+ Complexity    32624    31763     -861     
============================================
  Files          1877     1853      -24     
  Lines        139502   143391    +3889     
  Branches      15299    16279     +980     
============================================
+ Hits         102638   106308    +3670     
+ Misses        28908    28705     -203     
- Partials       7956     8378     +422     
Flag Coverage Δ
inttests 26.72% <43.33%> (+2.13%) ⬆️
systests 23.14% <43.33%> (-1.18%) ⬇️
unittests 73.67% <56.66%> (+0.82%) ⬆️

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

Files with missing lines Coverage Δ
...apache/bookkeeper/mledger/ManagedLedgerConfig.java 96.42% <100.00%> (+0.13%) ⬆️
...pache/bookkeeper/mledger/impl/RangeSetWrapper.java 81.25% <100.00%> (-13.09%) ⬇️
...org/apache/pulsar/broker/ServiceConfiguration.java 98.14% <100.00%> (-1.25%) ⬇️
...rg/apache/pulsar/broker/service/BrokerService.java 84.87% <100.00%> (+4.09%) ⬆️
...che/bookkeeper/mledger/impl/ManagedCursorImpl.java 79.30% <31.57%> (+<0.01%) ⬆️

... and 1017 files with indirect coverage changes

Comment on lines +2389 to +2393
if (position.getEntryId() == 0) {
previousPosition = PositionFactory.create(position.getLedgerId(), -1);
} else {
previousPosition = ledger.getPreviousPosition(position);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to move to the internal of the getPreviousPosition() method.

Copy link
Contributor Author

@poorbarcode poorbarcode Dec 27, 2024

Choose a reason for hiding this comment

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

It's better to move to the internal of the getPreviousPosition() method.

getPreviousPosition returns a meaningful position, in other words, it returns a position that exactly exists. And other methods rely on this feature, such as https://github.com/apache/pulsar/blob/master/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L3621-L3623. Only cursor individual ack range does not allow crossing ledgers so far.

if (specifiedLac.getEntryId() < 0) {
     // Calculate the meaningful LAC.
     Position actLac = getPreviousPosition(specifiedLac);
    ...
}

@FieldContext(
category = CATEGORY_STORAGE_ML,
doc = "Whether persist cursor ack stats as long arrays, which will compress the data and reduce GC rate")
private boolean managedLedgerPersistIndividualAckAsLongArray = false;
Copy link
Member

Choose a reason for hiding this comment

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

Does change the existing Pulsar 4.0 behavior when this defaults to false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, will rollback the default behavior to 3.0.x, in other words, removed the default behavior changes that introduced by #9292

@poorbarcode poorbarcode requested a review from lhotari December 30, 2024 02:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs ready-to-test release/4.0.2 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants