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 2021/08/21 17:08:49 UTC
[GitHub] [pulsar] codelipenghui opened a new pull request #11737: Fix the topic in fenced state and can not recover.
codelipenghui opened a new pull request #11737:
URL: https://github.com/apache/pulsar/pull/11737
Here is the log when the issue happens. The producer continues to reconnect to the broker, but the fenced state of the topic is always true.
```
19:01:42.351 [pulsar-io-4-1] INFO org.apache.pulsar.broker.service.ServerCnx - [/10.24.34.151:48052][persistent://public/default/test-8] Creating producer. producerId=8
19:01:42.352 [Thread-174681] INFO org.apache.pulsar.broker.service.ServerCnx - [/10.24.34.151:48052] persistent://public/default/test-8 configured with schema false
19:01:42.352 [Thread-174681] WARN org.apache.pulsar.broker.service.AbstractTopic - [persistent://public/default/test-8] Attempting to add producer to a fenced topic
19:01:42.352 [Thread-174681] ERROR org.apache.pulsar.broker.service.ServerCnx - [/10.24.34.151:48052] Failed to add producer to topic persistent://public/default/test-8: producerId=8, org.apache.pulsar.broker.service.BrokerServiceException$TopicFencedException: Topic is temporarily unavailable
```
After check the heap dump of the broker, the `pendingWriteOps` is 5, this is the reason why the topic can not recover from the fenced state.
The topic will change to unfenced only the `pendingWriteOps` is 0, details can find at [PersistentTopic.decrementPendingWriteOpsAndCheck()](https://github.com/apache/pulsar/blob/794aa20d9f2a4e668cc36465362d22e042e6e536/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java#L463)
But after checking the ML state of the topic, it shows the `pendingAddEntries` is 0 which not equals to `pendingWriteOps` of the topic.
The root cause is we are polling add entry op from the `pendingAddEntries` in multiple threads, one is the the ZK callback thread when complete the ledger creating (https://github.com/apache/pulsar/blob/794aa20d9f2a4e668cc36465362d22e042e6e536/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L1406, https://github.com/apache/pulsar/blob/794aa20d9f2a4e668cc36465362d22e042e6e536/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L1669),
another one is the ML worker thread when complete the add entry op (https://github.com/apache/pulsar/blob/794aa20d9f2a4e668cc36465362d22e042e6e536/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpAddEntry.java#L181)
After the ongoing add entry op complete, but the corresponding op might been polled by the `clearPendingAddEntries` method. So it will poll another one, but due to
not equals to the current op, the polled op will not get a chance to be failed, so that the `pendingWriteOps` will not change to 0.
I have attached the complete logs for the topic: [downloaded-logs-20210820-125853.csv](https://github.com/apache/pulsar/files/7025898/downloaded-logs-20210820-125853.csv)
The fix is to complete the add entry op with ManagedLedgerException if the polled op is not equals to the current op.
--
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] lhotari commented on a change in pull request #11737: Fix the topic in fenced state and can not recover.
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #11737:
URL: https://github.com/apache/pulsar/pull/11737#discussion_r693734024
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpAddEntry.java
##########
@@ -179,7 +179,16 @@ public void addComplete(int rc, final LedgerHandle lh, long entryId, Object ctx)
public void safeRun() {
// Remove this entry from the head of the pending queue
OpAddEntry firstInQueue = ml.pendingAddEntries.poll();
- checkArgument(this == firstInQueue);
+ if (firstInQueue == null) {
+ // The pending op might been polled by others such as cleanup pending op when create Ledger failed.
+ this.recycle();
+ return;
+ }
+ if (this != firstInQueue) {
+ firstInQueue.failed(new ManagedLedgerException("Unexpected add entry op when complete the add entry op."));
+ this.recycle();
+ return;
+ }
Review comment:
How about `ReferenceCountUtil.release(data);` in these cases? would that be needed to prevent memory leaks? `this.recycle` doesn't handle releasing of `data`.
--
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] lhotari commented on pull request #11737: Fix the topic in fenced state and can not recover.
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #11737:
URL: https://github.com/apache/pulsar/pull/11737#issuecomment-903492871
@codelipenghui Doesn't the problem impact 2.7.x too? Can we cherry-pick to 2.7.4 ?
--
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 change in pull request #11737: Fix the topic in fenced state and can not recover.
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #11737:
URL: https://github.com/apache/pulsar/pull/11737#discussion_r694673532
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpAddEntry.java
##########
@@ -179,7 +179,18 @@ public void addComplete(int rc, final LedgerHandle lh, long entryId, Object ctx)
public void safeRun() {
// Remove this entry from the head of the pending queue
OpAddEntry firstInQueue = ml.pendingAddEntries.poll();
- checkArgument(this == firstInQueue);
+ if (firstInQueue == null) {
+ // The pending op might been polled by others such as cleanup pending op when create Ledger failed.
+ ReferenceCountUtil.release(data);
+ this.recycle();
+ return;
+ }
+ if (this != firstInQueue) {
+ firstInQueue.failed(new ManagedLedgerException("Unexpected add entry op when complete the add entry op."));
+ ReferenceCountUtil.release(data);
+ this.recycle();
+ return;
+ }
Review comment:
Oh, I think I made a wrong change there, the `OpAddEntry.failed()` has released the data but looks like missed the object recycle, So we don't need to release it again.
So a thread takes the op from the queue, it should call the callback and release the data.
--
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] michaeljmarshall commented on a change in pull request #11737: Fix the topic in fenced state and can not recover.
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #11737:
URL: https://github.com/apache/pulsar/pull/11737#discussion_r694141533
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpAddEntry.java
##########
@@ -179,7 +179,18 @@ public void addComplete(int rc, final LedgerHandle lh, long entryId, Object ctx)
public void safeRun() {
// Remove this entry from the head of the pending queue
OpAddEntry firstInQueue = ml.pendingAddEntries.poll();
- checkArgument(this == firstInQueue);
+ if (firstInQueue == null) {
+ // The pending op might been polled by others such as cleanup pending op when create Ledger failed.
+ ReferenceCountUtil.release(data);
+ this.recycle();
+ return;
+ }
+ if (this != firstInQueue) {
+ firstInQueue.failed(new ManagedLedgerException("Unexpected add entry op when complete the add entry op."));
+ ReferenceCountUtil.release(data);
+ this.recycle();
+ return;
+ }
Review comment:
+1. @codelipenghui, can you explain why we call `firstInQueue.failed` and then release `this.data` and call `this.recycle()` without also calling `this.failed`? Note that the `failed` method has concurrency controls built in by using the `callbackUpdater`. Wouldn't we want to use those concurrency controls here as well?
```java
public void failed(ManagedLedgerException e) {
AddEntryCallback cb = callbackUpdater.getAndSet(this, null);
if (cb != null) {
ReferenceCountUtil.release(data);
cb.addFailed(e, ctx);
ml.mbean.recordAddEntryError();
}
}
```
--
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 change in pull request #11737: Fix the topic in fenced state and can not recover.
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #11737:
URL: https://github.com/apache/pulsar/pull/11737#discussion_r694887811
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpAddEntry.java
##########
@@ -179,7 +179,18 @@ public void addComplete(int rc, final LedgerHandle lh, long entryId, Object ctx)
public void safeRun() {
// Remove this entry from the head of the pending queue
OpAddEntry firstInQueue = ml.pendingAddEntries.poll();
- checkArgument(this == firstInQueue);
+ if (firstInQueue == null) {
+ // The pending op might been polled by others such as cleanup pending op when create Ledger failed.
+ ReferenceCountUtil.release(data);
+ this.recycle();
+ return;
+ }
+ if (this != firstInQueue) {
+ firstInQueue.failed(new ManagedLedgerException("Unexpected add entry op when complete the add entry op."));
+ ReferenceCountUtil.release(data);
+ this.recycle();
+ return;
+ }
Review comment:
Yes, clearPendingAddEntries is expected to call failed.
And yes no harm to call `this.failed`, I think we only complete the op which comes from the queue is more clear here. All the add entry ops will add the queue, so someone poll the op from the queue, it should make sure the `op.callback` has been called so that we don't need to care about `this.failed`, we just need to complete all the ops in the queue.
--
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] lhotari commented on a change in pull request #11737: Fix the topic in fenced state and can not recover.
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #11737:
URL: https://github.com/apache/pulsar/pull/11737#discussion_r694610235
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpAddEntry.java
##########
@@ -179,7 +179,18 @@ public void addComplete(int rc, final LedgerHandle lh, long entryId, Object ctx)
public void safeRun() {
// Remove this entry from the head of the pending queue
OpAddEntry firstInQueue = ml.pendingAddEntries.poll();
- checkArgument(this == firstInQueue);
+ if (firstInQueue == null) {
+ // The pending op might been polled by others such as cleanup pending op when create Ledger failed.
+ ReferenceCountUtil.release(data);
+ this.recycle();
+ return;
+ }
+ if (this != firstInQueue) {
+ firstInQueue.failed(new ManagedLedgerException("Unexpected add entry op when complete the add entry op."));
+ ReferenceCountUtil.release(data);
+ this.recycle();
+ return;
+ }
Review comment:
> there are 2 threads polling the add entry op from the queue
@codelipenghui The 2 threads polling seem to be part of the problem. Is that so?
One goal of the Managed Ledger design seems to be that there's a single thread per managed ledger executing operations. PR #11387 "Pin executor and scheduled executor threads for ManagedLedgerImpl" fixes some issues where this pinned executor solution wasn't used.
Just curious, would #11387 prevent the 2 threads polling the add entry op from the queue?
--
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 change in pull request #11737: Fix the topic in fenced state and can not recover.
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #11737:
URL: https://github.com/apache/pulsar/pull/11737#discussion_r694692995
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpAddEntry.java
##########
@@ -179,7 +179,18 @@ public void addComplete(int rc, final LedgerHandle lh, long entryId, Object ctx)
public void safeRun() {
// Remove this entry from the head of the pending queue
OpAddEntry firstInQueue = ml.pendingAddEntries.poll();
- checkArgument(this == firstInQueue);
+ if (firstInQueue == null) {
+ // The pending op might been polled by others such as cleanup pending op when create Ledger failed.
+ ReferenceCountUtil.release(data);
+ this.recycle();
+ return;
+ }
+ if (this != firstInQueue) {
+ firstInQueue.failed(new ManagedLedgerException("Unexpected add entry op when complete the add entry op."));
+ ReferenceCountUtil.release(data);
+ this.recycle();
+ return;
+ }
Review comment:
@lhotari Yes, I think run the `clearPendingAddEntries()` in the pinned executor can also fix the problem
--
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 change in pull request #11737: Fix the topic in fenced state and can not recover.
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #11737:
URL: https://github.com/apache/pulsar/pull/11737#discussion_r693928232
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpAddEntry.java
##########
@@ -179,7 +179,16 @@ public void addComplete(int rc, final LedgerHandle lh, long entryId, Object ctx)
public void safeRun() {
// Remove this entry from the head of the pending queue
OpAddEntry firstInQueue = ml.pendingAddEntries.poll();
- checkArgument(this == firstInQueue);
+ if (firstInQueue == null) {
+ // The pending op might been polled by others such as cleanup pending op when create Ledger failed.
+ this.recycle();
+ return;
+ }
+ if (this != firstInQueue) {
+ firstInQueue.failed(new ManagedLedgerException("Unexpected add entry op when complete the add entry op."));
+ this.recycle();
+ return;
+ }
Review comment:
Good point! Fixed.
--
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] michaeljmarshall commented on a change in pull request #11737: Fix the topic in fenced state and can not recover.
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #11737:
URL: https://github.com/apache/pulsar/pull/11737#discussion_r694472319
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpAddEntry.java
##########
@@ -179,7 +179,18 @@ public void addComplete(int rc, final LedgerHandle lh, long entryId, Object ctx)
public void safeRun() {
// Remove this entry from the head of the pending queue
OpAddEntry firstInQueue = ml.pendingAddEntries.poll();
- checkArgument(this == firstInQueue);
+ if (firstInQueue == null) {
+ // The pending op might been polled by others such as cleanup pending op when create Ledger failed.
+ ReferenceCountUtil.release(data);
+ this.recycle();
+ return;
+ }
+ if (this != firstInQueue) {
+ firstInQueue.failed(new ManagedLedgerException("Unexpected add entry op when complete the add entry op."));
+ ReferenceCountUtil.release(data);
+ this.recycle();
+ return;
+ }
Review comment:
Thank you for your clarification. I think I understand why we're calling `firstInQueue.failed`. I'm don't understand why we're calling `this.recycle` without also calling `addComplete` or `failed`. If I understand correctly, by calling `this.recycle`, we're going to set `callback = null;` without calling its callback. If my understanding is incorrect, can you explain why we're recycling `this` without calling its callback? Thank you.
--
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] lhotari commented on a change in pull request #11737: Fix the topic in fenced state and can not recover.
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #11737:
URL: https://github.com/apache/pulsar/pull/11737#discussion_r693982448
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpAddEntry.java
##########
@@ -179,7 +179,18 @@ public void addComplete(int rc, final LedgerHandle lh, long entryId, Object ctx)
public void safeRun() {
// Remove this entry from the head of the pending queue
OpAddEntry firstInQueue = ml.pendingAddEntries.poll();
- checkArgument(this == firstInQueue);
+ if (firstInQueue == null) {
+ // The pending op might been polled by others such as cleanup pending op when create Ledger failed.
+ ReferenceCountUtil.release(data);
+ this.recycle();
+ return;
+ }
+ if (this != firstInQueue) {
+ firstInQueue.failed(new ManagedLedgerException("Unexpected add entry op when complete the add entry op."));
+ ReferenceCountUtil.release(data);
+ this.recycle();
+ return;
+ }
Review comment:
I have one more question about this solution. Would it be necessary to call the callback for the OpAddEntry by using the "failed" method?
--
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 change in pull request #11737: Fix the topic in fenced state and can not recover.
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #11737:
URL: https://github.com/apache/pulsar/pull/11737#discussion_r694444126
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpAddEntry.java
##########
@@ -179,7 +179,18 @@ public void addComplete(int rc, final LedgerHandle lh, long entryId, Object ctx)
public void safeRun() {
// Remove this entry from the head of the pending queue
OpAddEntry firstInQueue = ml.pendingAddEntries.poll();
- checkArgument(this == firstInQueue);
+ if (firstInQueue == null) {
+ // The pending op might been polled by others such as cleanup pending op when create Ledger failed.
+ ReferenceCountUtil.release(data);
+ this.recycle();
+ return;
+ }
+ if (this != firstInQueue) {
+ firstInQueue.failed(new ManagedLedgerException("Unexpected add entry op when complete the add entry op."));
+ ReferenceCountUtil.release(data);
+ this.recycle();
+ return;
+ }
Review comment:
@lhotari @michaeljmarshall As I mentioned in the PR description, there are 2 threads polling the add entry op from the queue, but here poll out the op from the queue but does not call the callback method of the op, this will lead to the `pendingWriteOps` of the topic can't change to 0 (before adding an entry, pendingWriteOps++. after entry add success for failed, pendingWriteOps--), so we should call the callback of the op here to make sure the `pendingWriteOps` can change to 0. The root cause of the issue is we missed some op callback calls here because the first op in the queue is not equaled to the current OpAddEntry(the previous behavior just throws an exception). So It's not about concurrency controls of the `callbackUpdater`.
--
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 pull request #11737: Fix the topic in fenced state and can not recover.
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #11737:
URL: https://github.com/apache/pulsar/pull/11737#issuecomment-903722294
@lhotari I have added the label `release/2.7.4`.
--
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 change in pull request #11737: Fix the topic in fenced state and can not recover.
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #11737:
URL: https://github.com/apache/pulsar/pull/11737#discussion_r694658791
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpAddEntry.java
##########
@@ -179,7 +179,18 @@ public void addComplete(int rc, final LedgerHandle lh, long entryId, Object ctx)
public void safeRun() {
// Remove this entry from the head of the pending queue
OpAddEntry firstInQueue = ml.pendingAddEntries.poll();
- checkArgument(this == firstInQueue);
+ if (firstInQueue == null) {
+ // The pending op might been polled by others such as cleanup pending op when create Ledger failed.
+ ReferenceCountUtil.release(data);
+ this.recycle();
+ return;
+ }
+ if (this != firstInQueue) {
+ firstInQueue.failed(new ManagedLedgerException("Unexpected add entry op when complete the add entry op."));
+ ReferenceCountUtil.release(data);
+ this.recycle();
+ return;
+ }
Review comment:
@michaeljmarshall Do you mean the first if branch? another thread already poll the op out of the queue and call the callback of the op, see here https://github.com/apache/pulsar/blob/794aa20d9f2a4e668cc36465362d22e042e6e536/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L1406
--
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] hangc0276 merged pull request #11737: Fix the topic in fenced state and can not recover.
Posted by GitBox <gi...@apache.org>.
hangc0276 merged pull request #11737:
URL: https://github.com/apache/pulsar/pull/11737
--
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] michaeljmarshall commented on a change in pull request #11737: Fix the topic in fenced state and can not recover.
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on a change in pull request #11737:
URL: https://github.com/apache/pulsar/pull/11737#discussion_r694472319
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpAddEntry.java
##########
@@ -179,7 +179,18 @@ public void addComplete(int rc, final LedgerHandle lh, long entryId, Object ctx)
public void safeRun() {
// Remove this entry from the head of the pending queue
OpAddEntry firstInQueue = ml.pendingAddEntries.poll();
- checkArgument(this == firstInQueue);
+ if (firstInQueue == null) {
+ // The pending op might been polled by others such as cleanup pending op when create Ledger failed.
+ ReferenceCountUtil.release(data);
+ this.recycle();
+ return;
+ }
+ if (this != firstInQueue) {
+ firstInQueue.failed(new ManagedLedgerException("Unexpected add entry op when complete the add entry op."));
+ ReferenceCountUtil.release(data);
+ this.recycle();
+ return;
+ }
Review comment:
Thank you for your clarification. I think I understand why we're calling `firstInQueue.failed`. I don't understand why we're calling `this.recycle` without also calling `addComplete` or `failed`. If I understand correctly, by calling `this.recycle`, we're going to set `callback = null;` without calling its callback. If my understanding is incorrect, can you explain why we're recycling `this` without calling its callback? Thank you.
--
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] lhotari commented on pull request #11737: Fix the topic in fenced state and can not recover.
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #11737:
URL: https://github.com/apache/pulsar/pull/11737#issuecomment-903492015
@codelipenghui Do you think that this PR could fix #5284 ?
--
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] lhotari commented on a change in pull request #11737: Fix the topic in fenced state and can not recover.
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #11737:
URL: https://github.com/apache/pulsar/pull/11737#discussion_r694811827
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpAddEntry.java
##########
@@ -179,7 +179,18 @@ public void addComplete(int rc, final LedgerHandle lh, long entryId, Object ctx)
public void safeRun() {
// Remove this entry from the head of the pending queue
OpAddEntry firstInQueue = ml.pendingAddEntries.poll();
- checkArgument(this == firstInQueue);
+ if (firstInQueue == null) {
+ // The pending op might been polled by others such as cleanup pending op when create Ledger failed.
+ ReferenceCountUtil.release(data);
+ this.recycle();
+ return;
+ }
+ if (this != firstInQueue) {
+ firstInQueue.failed(new ManagedLedgerException("Unexpected add entry op when complete the add entry op."));
+ ReferenceCountUtil.release(data);
+ this.recycle();
+ return;
+ }
Review comment:
> Oh, I think I made a wrong change there, the OpAddEntry.failed() has released the data but looks like missed the object recycle, So we don't need to release it again.
>
> So a thread takes the op from the queue, it should call the callback and release the data.
@codelipenghui Why isn't `this.failed(...)` called when `this != firstInQueue` ?
I would be expecting something like this:
```java
if (this != firstInQueue) {
if (firstInQueue != null) {
firstInQueue.failed(
new ManagedLedgerException("Unexpected add entry op when complete the add entry op."));
}
this.failed(new ManagedLedgerException("Unexpected add entry op when complete the add entry op."));
return;
}
```
Is there a reason to _not_ call `this.failed`?
--
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] lhotari commented on a change in pull request #11737: Fix the topic in fenced state and can not recover.
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #11737:
URL: https://github.com/apache/pulsar/pull/11737#discussion_r694825709
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpAddEntry.java
##########
@@ -179,7 +179,18 @@ public void addComplete(int rc, final LedgerHandle lh, long entryId, Object ctx)
public void safeRun() {
// Remove this entry from the head of the pending queue
OpAddEntry firstInQueue = ml.pendingAddEntries.poll();
- checkArgument(this == firstInQueue);
+ if (firstInQueue == null) {
+ // The pending op might been polled by others such as cleanup pending op when create Ledger failed.
+ ReferenceCountUtil.release(data);
+ this.recycle();
+ return;
+ }
+ if (this != firstInQueue) {
+ firstInQueue.failed(new ManagedLedgerException("Unexpected add entry op when complete the add entry op."));
+ ReferenceCountUtil.release(data);
+ this.recycle();
+ return;
+ }
Review comment:
@codelipenghui reading your previous answer, I would understand that clearPendingAddEntries is expected to call failed?
Since `failed` method has concurrency controls, I think it wouldn't cause any harm to call `this.failed` when `this != firstInQueue`. No?
--
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