You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "poorbarcode (via GitHub)" <gi...@apache.org> on 2023/06/12 11:16:36 UTC
[GitHub] [pulsar] poorbarcode opened a new pull request, #20561: [fix][broker] Remove failed future when topic load error
poorbarcode opened a new pull request, #20561:
URL: https://github.com/apache/pulsar/pull/20561
### Motivation
When a topic load fails due to `timeout ex`, Broker will not clean up the exception future, causing other APIs to fail when they touch this future. You can reproduce it by `testUnloadNamespaceAfterLoadTopicFailed`
### Modifications
Remove failed future when topic load error
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
### Matching PR in forked repository
PR in forked repository: x
--
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] poorbarcode commented on a diff in pull request #20561: [fix][broker] Remove failed future when topic load error
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #20561:
URL: https://github.com/apache/pulsar/pull/20561#discussion_r1226619706
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -1091,6 +1091,9 @@ public CompletableFuture<Optional<Topic>> getTopic(final TopicName topicName, bo
}
});
}
+ final CompletableFuture<Optional<Topic>> topicFutureToRemove = topicFuture;
+ topicFuture.whenComplete((ignore, ex) -> topics.remove(topicName.toString(), topicFutureToRemove));
Review Comment:
@eolivelli
Ah, sorry. I lost a check: `if (ex != null)`. It only is removed if the future was failed, and we call `remove(k, v)`, and this method will not do incorrect deletions. Could you take a look again?
--
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 #20561: [fix][broker] Remove failed future when topic load error
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #20561:
URL: https://github.com/apache/pulsar/pull/20561#issuecomment-1700225423
The pr had no activity for 30 days, mark with Stale label.
--
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
Re: [PR] [fix][broker] Remove failed future when topic load error [pulsar]
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat commented on PR #20561:
URL: https://github.com/apache/pulsar/pull/20561#issuecomment-6355587631
Thanks @poorbarcode, and sorry this sat so long. Since this was opened, `getTopic` has been restructured: every load failure now goes through `failTopicFuture`, which evicts the future, and #20540 (merged today) evicts timed-out loads once their pending actions finish. Unload also tolerates failed futures now. I'm closing this as superseded. One path is still left: a queued load whose ownership check fails in `createPendingLoadTopic` isn't evicted. I'll follow up on that separately. If you see stale failures on current master, please reopen with a repro.
--
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
Re: [PR] [fix][broker] Remove failed future when topic load error [pulsar]
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat closed pull request #20561: [fix][broker] Remove failed future when topic load error
URL: https://github.com/apache/pulsar/pull/20561
--
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 #20561: [fix][broker] Remove failed future when topic load error
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on code in PR #20561:
URL: https://github.com/apache/pulsar/pull/20561#discussion_r1226596488
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -1091,6 +1091,9 @@ public CompletableFuture<Optional<Topic>> getTopic(final TopicName topicName, bo
}
});
}
+ final CompletableFuture<Optional<Topic>> topicFutureToRemove = topicFuture;
+ topicFuture.whenComplete((ignore, ex) -> topics.remove(topicName.toString(), topicFutureToRemove));
Review Comment:
please note that you are appending a new whenComplete listener every time this method is called and the object is already in the map.
we have to call "whenComplete" only if the body of the lambda expression in computeIfAbsent is executed.
I know that it sounds ugly, but we must to it
--
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 #20561: [fix][broker] Remove failed future when topic load error
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on code in PR #20561:
URL: https://github.com/apache/pulsar/pull/20561#discussion_r1227228326
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -1091,6 +1091,9 @@ public CompletableFuture<Optional<Topic>> getTopic(final TopicName topicName, bo
}
});
}
+ final CompletableFuture<Optional<Topic>> topicFutureToRemove = topicFuture;
+ topicFuture.whenComplete((ignore, ex) -> topics.remove(topicName.toString(), topicFutureToRemove));
Review Comment:
The problem is that we call whenComplete every time we invoke this method and only the first time we create the object and add it to the Map.
So everytime we enqueue the execution. If this method is called in an hotpath you will see many allocations
--
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 #20561: [fix][broker] Remove failed future when topic load error
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #20561:
URL: https://github.com/apache/pulsar/pull/20561#issuecomment-1622129562
this is already covered in #20540 which was created before this PR. It uses a different approach in the `failTopicFuture` and `completeTopicFuture` method.
I agree that the approach in this PR could resolve the particular issue, but #20540 targets to cover also the cases where the bundle gets unloaded or the broker gets shutdown while a topic is loading.
--
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] poorbarcode commented on a diff in pull request #20561: [fix][broker] Remove failed future when topic load error
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #20561:
URL: https://github.com/apache/pulsar/pull/20561#discussion_r1227680631
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -1091,6 +1091,9 @@ public CompletableFuture<Optional<Topic>> getTopic(final TopicName topicName, bo
}
});
}
+ final CompletableFuture<Optional<Topic>> topicFutureToRemove = topicFuture;
+ topicFuture.whenComplete((ignore, ex) -> topics.remove(topicName.toString(), topicFutureToRemove));
Review Comment:
@eolivelli
You are right. Thanks, already 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