[improve][broker] Recycle OpReadEntry in some corner cases #16399
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BewareMyPower
added
type/bug
BewareMyPower
requested review from
315157973,
Jason918,
codelipenghui,
congbobo184,
eolivelli,
gaozhangmin and
merlimat
|
@BewareMyPower Please provide a correct documentation label for your PR. |
github-actions
bot
added
doc-label-missing
doc-not-needed
merlimat
approved these changes
Jul 5, 2022
Contributor
merlimat
left a comment
merlimat
left a comment
There was a problem hiding this comment.
👍 I wouldn't mark it as a bug though. It's ok to skip recycling for error condition. It wil just cause the object to be GCed instead of going back to the pool.
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java
Outdated
Show resolved
Hide resolved
BewareMyPower
changed the title
BewareMyPower
added
type/enhancement
BewareMyPower
changed the title
codelipenghui
approved these changes
Jul 6, 2022
eolivelli
requested changes
Jul 6, 2022
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java
Outdated
Show resolved
Hide resolved
### Motivation `ManagedCursorImpl` maintains a field `waitingReadOp` as the cache of the `OpReadEntry` created in `asyncReadEntriesOrWait` when there are no more entries to read. However, there are two cases that the created `OpReadEntry` are not recycled: 1. `asyncReadEntriesOrWait` is called repeatedly when `waitingReadOp` is not null and there are no more entries. The new created `OpReadEntry` cannot pass the CAS check but it's not recycled. 2. `cancelPendingReadRequest` is called. The `waitingReadOp` is just set with null and the previous reference is not recycled. ### Modifications For the two cases described above, recycle the `OpReadEntry` objects. ### Verifying this change To verify all `OpReadEntry` are recycled correctly, this PRs adds two static atomic integers to record the create count and the recycle count. Then add `testOpReadEntryRecycle` to reproduce the corner cases and verify these counts.
BewareMyPower
force-pushed
the
bewaremypower/op-read-entry-recycle
branch
from
b8d77c8 to
7232f22
Compare
codelipenghui
pushed a commit
that referenced
this pull request
Jul 10, 2022
### Motivation `ManagedCursorImpl` maintains a field `waitingReadOp` as the cache of the `OpReadEntry` created in `asyncReadEntriesOrWait` when there are no more entries to read. However, there are two cases that the created `OpReadEntry` are not recycled: 1. `asyncReadEntriesOrWait` is called repeatedly when `waitingReadOp` is not null and there are no more entries. The new created `OpReadEntry` cannot pass the CAS check but it's not recycled. 2. `cancelPendingReadRequest` is called. The `waitingReadOp` is just set with null and the previous reference is not recycled. ### Modifications For the two cases described above, recycle the `OpReadEntry` objects. ### Verifying this change Add `testOpReadEntryRecycle` to reproduce the corner cases and verify the count of `recycle()` calls. (cherry picked from commit 6cec62e)
zymap
pushed a commit
to zymap/pulsar
that referenced
this pull request
Jul 11, 2022
) ### Motivation `ManagedCursorImpl` maintains a field `waitingReadOp` as the cache of the `OpReadEntry` created in `asyncReadEntriesOrWait` when there are no more entries to read. However, there are two cases that the created `OpReadEntry` are not recycled: 1. `asyncReadEntriesOrWait` is called repeatedly when `waitingReadOp` is not null and there are no more entries. The new created `OpReadEntry` cannot pass the CAS check but it's not recycled. 2. `cancelPendingReadRequest` is called. The `waitingReadOp` is just set with null and the previous reference is not recycled. ### Modifications For the two cases described above, recycle the `OpReadEntry` objects. ### Verifying this change Add `testOpReadEntryRecycle` to reproduce the corner cases and verify the count of `recycle()` calls.
nicoloboschi
pushed a commit
to datastax/pulsar
that referenced
this pull request
Jul 11, 2022
) ### Motivation `ManagedCursorImpl` maintains a field `waitingReadOp` as the cache of the `OpReadEntry` created in `asyncReadEntriesOrWait` when there are no more entries to read. However, there are two cases that the created `OpReadEntry` are not recycled: 1. `asyncReadEntriesOrWait` is called repeatedly when `waitingReadOp` is not null and there are no more entries. The new created `OpReadEntry` cannot pass the CAS check but it's not recycled. 2. `cancelPendingReadRequest` is called. The `waitingReadOp` is just set with null and the previous reference is not recycled. ### Modifications For the two cases described above, recycle the `OpReadEntry` objects. ### Verifying this change Add `testOpReadEntryRecycle` to reproduce the corner cases and verify the count of `recycle()` calls. (cherry picked from commit 6cec62e) (cherry picked from commit 7c37e56)
wuxuanqicn
pushed a commit
to wuxuanqicn/pulsar
that referenced
this pull request
Jul 14, 2022
) ### Motivation `ManagedCursorImpl` maintains a field `waitingReadOp` as the cache of the `OpReadEntry` created in `asyncReadEntriesOrWait` when there are no more entries to read. However, there are two cases that the created `OpReadEntry` are not recycled: 1. `asyncReadEntriesOrWait` is called repeatedly when `waitingReadOp` is not null and there are no more entries. The new created `OpReadEntry` cannot pass the CAS check but it's not recycled. 2. `cancelPendingReadRequest` is called. The `waitingReadOp` is just set with null and the previous reference is not recycled. ### Modifications For the two cases described above, recycle the `OpReadEntry` objects. ### Verifying this change Add `testOpReadEntryRecycle` to reproduce the corner cases and verify the count of `recycle()` calls.
mattisonchao
pushed a commit
that referenced
this pull request
Jul 15, 2022
### Motivation `ManagedCursorImpl` maintains a field `waitingReadOp` as the cache of the `OpReadEntry` created in `asyncReadEntriesOrWait` when there are no more entries to read. However, there are two cases that the created `OpReadEntry` are not recycled: 1. `asyncReadEntriesOrWait` is called repeatedly when `waitingReadOp` is not null and there are no more entries. The new created `OpReadEntry` cannot pass the CAS check but it's not recycled. 2. `cancelPendingReadRequest` is called. The `waitingReadOp` is just set with null and the previous reference is not recycled. ### Modifications For the two cases described above, recycle the `OpReadEntry` objects. ### Verifying this change Add `testOpReadEntryRecycle` to reproduce the corner cases and verify the count of `recycle()` calls. (cherry picked from commit 6cec62e)
BewareMyPower
added a commit
that referenced
this pull request
Jul 29, 2022
4 tasks
Contributor
Author
|
Move the |
BewareMyPower
added a commit
that referenced
this pull request
Aug 1, 2022
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
ManagedCursorImplmaintains a fieldwaitingReadOpas the cache ofthe
OpReadEntrycreated inasyncReadEntriesOrWaitwhen there are nomore entries to read. However, there are two cases that the created
OpReadEntryare not recycled:asyncReadEntriesOrWaitis called repeatedly whenwaitingReadOpisnot null and there are no more entries. The new created
OpReadEntrycannot pass the CAS check but it's not recycled.
cancelPendingReadRequestis called. ThewaitingReadOpis just setwith null and the previous reference is not recycled.
Modifications
For the two cases described above, recycle the
OpReadEntryobjects.Verifying this change
Add
testOpReadEntryRecycleto reproduce the corner cases and verify thecount of
recycle()calls.Documentation
Check the box below or label this PR directly.
Need to update docs?
doc-required(Your PR needs to update docs and you will update later)
doc-not-needed(Please explain why)
doc(Your PR contains doc changes)
doc-complete(Docs have been already added)