-
Notifications
You must be signed in to change notification settings - Fork 953
Fix check read failed entry memory leak issue. #4513
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
base: master
Are you sure you want to change the base?
Conversation
@@ -351,6 +352,8 @@ private boolean tryReadingFaultyEntries(LedgerHandle lh, LedgerFragment ledgerFr | |||
lh.asyncReadEntries(entryIdToRead, entryIdToRead, (rc, ledHan, seq, ctx) -> { | |||
long thisEntryId = (Long) ctx; | |||
if (rc == BKException.Code.OK) { | |||
LedgerEntry entry = seq.nextElement(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible for seq
to have multiple elements? It would be better to check with hasMoreElements
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
- hasMoreElements needs to be judged.
- at the same time, if (rc == BKException.Code.OK) is true or not, this release needs to be added.
- then I suggest adding a testcase to cover it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible for seq to have multiple elements? It would be better to check with hasMoreElements here.
Make sense.
at the same time, if (rc == BKException.Code.OK) is true or not, this release needs to be added.
For this case, if the rc == BKException.Code.OK
is false, the seq will be null, we don't need to release the seq.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good jobs
When checking the read failed entry, it doesn't release the data when read successfully.