Skip to content

#4574: fix sanity check asyncBatchReadEntries #4579

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

Conversation

mazzucchi-andrea
Copy link
Contributor

Fix: #4574

Motivation

The little sanity check in the method asyncBatchReadEntries in LedgerHandle does not check if the entrId is equal to or grater
to zero.

Changes

The method validates the input only for startEntry > lastAddConfirmed. This fix makes it compliant to the other reader methods and validates the input like this startEntry < 0 || startEntry > lastAddConfirmed.

Copy link
Member

@StevenLuMT StevenLuMT left a comment

Choose a reason for hiding this comment

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

Good jobs

@StevenLuMT
Copy link
Member

StevenLuMT commented Apr 15, 2025

@mazzucchi-andrea If possible, I think I'd add a startEntry < 0 exception test case

Copy link
Contributor

@xiezhx9 xiezhx9 left a comment

Choose a reason for hiding this comment

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

IncorrectParameter seems more precise here

Copy link
Member

@hezhangjian hezhangjian left a comment

Choose a reason for hiding this comment

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

Can startEntry less than zero?

Copy link
Member

@hezhangjian hezhangjian left a comment

Choose a reason for hiding this comment

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

LGTM

@mazzucchi-andrea
Copy link
Contributor Author

@mazzucchi-andrea If possible, I think I'd add a startEntry < 0 exception test case

Done!

@StevenLuMT StevenLuMT merged commit 3fea440 into apache:master Apr 15, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

batchReadEntries raises an unexpected exception
4 participants