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][PIP-327] fix recover from ledger when ledgerForceRecovery is true #23426

Merged
merged 1 commit into from
Oct 9, 2024

Conversation

summeriiii
Copy link
Contributor

Fixes #23425

Motivation

This issue was introduced by PIP-327(#21759). when we set config ledgerForceRecovery true, the following judgment in method ManagedCursorImpl#recoverFromLedger is always true, which leads to skipping the correct logic behind it.

           if (isBkErrorNotRecoverable(rc) || ledgerForceRecovery) {
               log.error("[{}] Error opening metadata ledger {} for cursor {}: {}", ledger.getName(), ledgerId, name,
                       BKException.getMessage(rc));
               // Rewind to oldest entry available
               initialize(getRollbackPosition(info), Collections.emptyMap(), cursorProperties, callback);
               return;
           } 

Modifications

  • change the condition to rc != BKException.Code.OK && ledgerForceRecovery
          if (isBkErrorNotRecoverable(rc) || (rc != BKException.Code.OK && ledgerForceRecovery)) {
               log.error("[{}] Error opening metadata ledger {} for cursor {}: {}", ledger.getName(), ledgerId, name,
                       BKException.getMessage(rc));
               // Rewind to oldest entry available
               initialize(getRollbackPosition(info), Collections.emptyMap(), cursorProperties, callback);
               return;
           }

Documentation

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

Matching PR in forked repository

PR in forked repository:

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Oct 9, 2024
@summeriiii summeriiii changed the title [fix][ml][PIP-327] fix recover from ledger when ledgerForceRecovery i… [fix][ml][PIP-327] fix recover from ledger when ledgerForceRecovery is true Oct 9, 2024
@lhotari lhotari added release/blocker Indicate the PR or issue that should block the release until it gets resolved ready-to-test labels Oct 9, 2024
@lhotari lhotari added this to the 4.0.0 milestone Oct 9, 2024
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

LGTM. Good work @summeriiii!

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 74.30%. Comparing base (bbc6224) to head (79321b4).
Report is 650 commits behind head on master.

Files with missing lines Patch % Lines
...che/bookkeeper/mledger/impl/ManagedCursorImpl.java 0.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #23426      +/-   ##
============================================
+ Coverage     73.57%   74.30%   +0.72%     
- Complexity    32624    34354    +1730     
============================================
  Files          1877     1949      +72     
  Lines        139502   146867    +7365     
  Branches      15299    16168     +869     
============================================
+ Hits         102638   109124    +6486     
- Misses        28908    29335     +427     
- Partials       7956     8408     +452     
Flag Coverage Δ
inttests 27.72% <0.00%> (+3.14%) ⬆️
systests 24.36% <0.00%> (+0.04%) ⬆️
unittests 73.66% <0.00%> (+0.81%) ⬆️

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

Files with missing lines Coverage Δ
...che/bookkeeper/mledger/impl/ManagedCursorImpl.java 79.64% <0.00%> (+0.34%) ⬆️

... and 630 files with indirect coverage changes

@lhotari lhotari merged commit ed01b0e into apache:master Oct 9, 2024
57 of 58 checks passed
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/blocker Indicate the PR or issue that should block the release until it gets resolved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] recover from ledger incorrect when set config ledgerForceRecovery true
3 participants