You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/07/05 10:02:36 UTC
[GitHub] [pulsar] BewareMyPower opened a new pull request, #16399: [fix][broker] OpReadEntry is not recycled in some corner cases
BewareMyPower opened a new pull request, #16399:
URL: https://github.com/apache/pulsar/pull/16399
### 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.
### 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)
- [x] `doc-not-needed`
(Please explain why)
- [ ] `doc`
(Your PR contains doc changes)
- [ ] `doc-complete`
(Docs have been already added)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower merged pull request #16399: [improve][broker] Recycle OpReadEntry in some corner cases
Posted by GitBox <gi...@apache.org>.
BewareMyPower merged PR #16399:
URL: https://github.com/apache/pulsar/pull/16399
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a diff in pull request #16399: [improve][broker] Recycle OpReadEntry in some corner cases
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #16399:
URL: https://github.com/apache/pulsar/pull/16399#discussion_r914648887
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java:
##########
@@ -34,6 +36,9 @@
class OpReadEntry implements ReadEntriesCallback {
+ private static AtomicInteger createCount = null;
Review Comment:
we should do this with PowerMock
please do not add such instrumentation only for testing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #16399: [improve][broker] Recycle OpReadEntry in some corner cases
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #16399:
URL: https://github.com/apache/pulsar/pull/16399#discussion_r914834696
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java:
##########
@@ -34,6 +36,9 @@
class OpReadEntry implements ReadEntriesCallback {
+ private static AtomicInteger createCount = null;
Review Comment:
Done. PTAL again. BTW, I found Mockito can do that as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on pull request #16399: [improve][broker] Recycle OpReadEntry in some corner cases
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on PR #16399:
URL: https://github.com/apache/pulsar/pull/16399#issuecomment-1199040441
Move the `release/2.8.4` label to https://github.com/apache/pulsar/pull/16869
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] BewareMyPower commented on a diff in pull request #16399: [improve][broker] Recycle OpReadEntry in some corner cases
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #16399:
URL: https://github.com/apache/pulsar/pull/16399#discussion_r914357537
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java:
##########
@@ -48,6 +53,9 @@ class OpReadEntry implements ReadEntriesCallback {
public static OpReadEntry create(ManagedCursorImpl cursor, PositionImpl readPositionRef, int count,
ReadEntriesCallback callback, Object ctx, PositionImpl maxPosition) {
OpReadEntry op = RECYCLER.get();
+ if (createCount != null) {
Review Comment:
I just removed the `volatile` instead. In non-testing cases, these two variables are always null and never changed and we don't have to guarantee the visibility. Now, the overhead is just a conditional check on normal variables, just like `if (log.isDebugEnabled())` in other places.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on a diff in pull request #16399: [fix][broker] OpReadEntry is not recycled in some corner cases
Posted by GitBox <gi...@apache.org>.
merlimat commented on code in PR #16399:
URL: https://github.com/apache/pulsar/pull/16399#discussion_r914248637
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java:
##########
@@ -48,6 +53,9 @@ class OpReadEntry implements ReadEntriesCallback {
public static OpReadEntry create(ManagedCursorImpl cursor, PositionImpl readPositionRef, int count,
ReadEntriesCallback callback, Object ctx, PositionImpl maxPosition) {
OpReadEntry op = RECYCLER.get();
+ if (createCount != null) {
Review Comment:
This will introduce a `volatile` read in the prod code which is not free. I'd avoid it if the purpose is for testing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a diff in pull request #16399: [improve][broker] Recycle OpReadEntry in some corner cases
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #16399:
URL: https://github.com/apache/pulsar/pull/16399#discussion_r914650032
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java:
##########
@@ -34,6 +36,9 @@
class OpReadEntry implements ReadEntriesCallback {
+ private static AtomicInteger createCount = null;
Review Comment:
with PowerMock you can wrap the calls to create and to recycle
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #16399: [fix][broker] OpReadEntry is not recycled in some corner cases
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #16399:
URL: https://github.com/apache/pulsar/pull/16399#issuecomment-1174874953
@BewareMyPower Please provide a correct documentation label for your PR.
Instructions see [Pulsar Documentation Label Guide](https://docs.google.com/document/d/1Qw7LHQdXWBW9t2-r-A7QdFDBwmZh6ytB4guwMoXHqc0).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a diff in pull request #16399: [fix][broker] OpReadEntry is not recycled in some corner cases
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #16399:
URL: https://github.com/apache/pulsar/pull/16399#discussion_r914339409
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpReadEntry.java:
##########
@@ -48,6 +53,9 @@ class OpReadEntry implements ReadEntriesCallback {
public static OpReadEntry create(ManagedCursorImpl cursor, PositionImpl readPositionRef, int count,
ReadEntriesCallback callback, Object ctx, PositionImpl maxPosition) {
OpReadEntry op = RECYCLER.get();
+ if (createCount != null) {
Review Comment:
Maybe we can try to recycle the OpReadEntry again in the test to confirm the OpReadEntry is recycled? So that we don't need to introduce this operation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org