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

AutoRecovery supports batch read #4211

Merged

Conversation

hangc0276
Copy link
Contributor

Motivation

We support batch reading in BP-62 and auto-recovery is the best case to support this feature. This PR supports batch reading for auto-recovery and introduces a flag recoveryBatchReadEnabled to enable batch reading for auto-recovery

Changes

We introduce a flag recoveryBatchReadEnabled to enable batch reading for auto-recovery.

@hangc0276 hangc0276 changed the title AutoRecovery support batch read AutoRecovery supports batch read Feb 19, 2024
@hangc0276 hangc0276 self-assigned this Feb 19, 2024
@hangc0276 hangc0276 added this to the 4.17.0 milestone Feb 19, 2024
@hangc0276
Copy link
Contributor Author

@horizonzy I introduced batch reading for auto-recovery, please help take a look, thanks.

}
toSend.release();
}
if (lastEntryId == endEntryId) {
Copy link
Member

Choose a reason for hiding this comment

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

When lastEntryId == endEntryId, we needn't callback ledgerFragmentMcb.processResult(BKException.Code.OK, null, null);.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! Updated

LOG.error("BK error reading ledger entries: {} - {}",
startEntryId, endEntryId, BKException.create(rc));
onReadEntryFailureCallback.accept(lh.getId(), startEntryId);
ledgerFragmentMcb.processResult(rc, null, null);
Copy link
Member

Choose a reason for hiding this comment

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

The ledgerFragmentMcb is MultiCallback, when batch read fails, the processResult invoke count may be not enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Updated.

Copy link
Member

@horizonzy horizonzy left a comment

Choose a reason for hiding this comment

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

LGTM.

@merlimat merlimat merged commit ef4ce13 into apache:master Feb 20, 2024
20 checks passed
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.

None yet

4 participants