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/04/12 16:13:21 UTC
[GitHub] [pulsar] AnonHxy opened a new pull request, #15139: [fix][broker]Fix create topic fail deadlock
AnonHxy opened a new pull request, #15139:
URL: https://github.com/apache/pulsar/pull/15139
Fixs #15131
### Motivation
Fix #15131
### Modifications
`topics` remove in another thread
### Verifying this change
- [x] Make sure that the change passes the CI checks.
### Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): (no)
- The public API: (no)
- The schema: (no)
- The default values of configurations: (no)
- The wire protocol: (no)
- The rest endpoints: (no)
- The admin cli options: (no)
- Anything that affects deployment: (no)
### Documentation
- [x] `no-need-doc`
(Please explain why)
--
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] gaozhangmin commented on pull request #15139: [fix][broker]Fix create topic fail deadlock
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on PR #15139:
URL: https://github.com/apache/pulsar/pull/15139#issuecomment-1097495881
LGTM, I'd like to point out that `createNonPersistentTopic` had fix this bug already.
https://github.com/apache/pulsar/blob/a4c4aea993aabab5231d4136b7eba366bee9e778/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java#L1166-L1171
--
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] Shawyeok commented on pull request #15139: [fix][broker]Fix create topic fail deadlock
Posted by GitBox <gi...@apache.org>.
Shawyeok commented on PR #15139:
URL: https://github.com/apache/pulsar/pull/15139#issuecomment-1098692062
I've see many exception branch ends with a line: `pulsar.getExecutor().execute(() -> topics.remove(topic, topicFuture));`, that's weird by the way.
It seems just a cleanup operation and it could be handled in `only one place`, such as below:
```diff
Index: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
--- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java (revision 5433d2ea03ff97a688e9c493e54282e1df07d66d)
+++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java (date 1649909372658)
@@ -977,12 +977,13 @@
}
}
final boolean isPersistentTopic = TopicName.get(topic).getDomain().equals(TopicDomain.persistent);
+ CompletableFuture<Optional<Topic>> loadTopicFuture;
if (isPersistentTopic) {
- return topics.computeIfAbsent(topic, (topicName) -> {
+ loadTopicFuture = topics.computeIfAbsent(topic, (topicName) -> {
return this.loadOrCreatePersistentTopic(topicName, createIfMissing, properties);
});
} else {
- return topics.computeIfAbsent(topic, (name) -> {
+ loadTopicFuture = topics.computeIfAbsent(topic, (name) -> {
final TopicName topicName = TopicName.get(name);
if (topicName.isPartitioned()) {
final TopicName partitionedTopicName = TopicName.get(topicName.getPartitionedTopicName());
@@ -999,6 +1000,12 @@
}
});
}
+ return loadTopicFuture.exceptionally(ignore -> {
+ // wo don't need remove entry in another thread any more
+ // cause we are not in the computeIfAbsent block
+ topics.remove(topic, loadTopicFuture);
+ return null;
+ });
} catch (IllegalArgumentException e) {
log.warn("[{}] Illegalargument exception when loading topic", topic, e);
return FutureUtil.failedFuture(e);
```
--
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] Jason918 commented on pull request #15139: [fix][broker]Fix create topic fail deadlock
Posted by GitBox <gi...@apache.org>.
Jason918 commented on PR #15139:
URL: https://github.com/apache/pulsar/pull/15139#issuecomment-1100850594
/pulsarbot run-failure-checks
--
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] gaozhangmin commented on a diff in pull request #15139: [fix][broker]Fix create topic fail deadlock
Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on code in PR #15139:
URL: https://github.com/apache/pulsar/pull/15139#discussion_r849028068
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -1443,10 +1443,10 @@ public void openLedgerComplete(ManagedLedger ledger, Object ctx) {
.exceptionally((ex) -> {
log.warn("Replication or dedup check failed."
+ " Removing topic from topics list {}, {}", topic, ex);
- persistentTopic.stopReplProducers().whenComplete((v, exception) -> {
Review Comment:
We don't need change here, because it already completed with exception. the write lock is released.
--
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] AnonHxy commented on pull request #15139: [fix][broker]Fix create topic fail deadlock
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #15139:
URL: https://github.com/apache/pulsar/pull/15139#issuecomment-1098221083
> In order to avoid the same problem, do we need to improve `ConcurrentOpenHashMap`?
>
> In `ConcurrentHashMap`, The same operation throws an exception, which makes it easier to spot the problem.
>
> For example below:
>
> ```
> Map<String, String> map = new ConcurrentHashMap<>();
>
> map.computeIfAbsent("test", (key) -> {
> map.remove("test");
> return "ok";
> });
> ```
>
> It will throw `java.lang.IllegalStateException: Recursive update`
That's great. But it will introduce a new problem. For example the following code:
```
persistentTopic.stopReplProducers().whenComplete((v, exception) -> {
topics.remove(topic, topicFuture);
topicFuture.completeExceptionally(ex);
})
```
If `topics.remove` throw exceptions, the topicFuture will not be done. So we need check the existed code like this and also avoid in the future. So in my opinion， we'd better keep it the way it is.@shibd
--
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] AnonHxy closed pull request #15139: [fix][broker]Fix create topic fail deadlock
Posted by GitBox <gi...@apache.org>.
AnonHxy closed pull request #15139: [fix][broker]Fix create topic fail deadlock
URL: https://github.com/apache/pulsar/pull/15139
--
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] AnonHxy commented on pull request #15139: [fix][broker]Fix create topic fail deadlock
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #15139:
URL: https://github.com/apache/pulsar/pull/15139#issuecomment-1118637729
/pulsarbot run-failure-checks
--
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] AnonHxy commented on pull request #15139: [fix][broker]Fix create topic fail deadlock
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #15139:
URL: https://github.com/apache/pulsar/pull/15139#issuecomment-1102247796
Fix the `ConcurrentOpenHashMap` in the latest commit , @Jason918 @gaozhangmin @shibd @eolivelli @Shawyeok PTAL
--
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 pull request #15139: [fix][broker]Fix create topic fail deadlock
Posted by GitBox <gi...@apache.org>.
eolivelli commented on PR #15139:
URL: https://github.com/apache/pulsar/pull/15139#issuecomment-1106077001
in theory we should follow the semantics of computeIfAbsent, that is...if the operation fails we should return null, and not try to remove something that is not in the map
what about this:
```
if (topics.contains(topic)) {
topics.remove(topic, topicFuture);
}
```
so, if we are in the case of "computeIfAbsent" when the topicFuture has not been added to the map, we do nothing.
if we are in other code paths, in which the topic is already in the map, we do the remove 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
[GitHub] [pulsar] AnonHxy commented on pull request #15139: [fix][broker]Fix create topic fail deadlock
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #15139:
URL: https://github.com/apache/pulsar/pull/15139#issuecomment-1119200983
Add UT, PTAL @eolivelli @codelipenghui
--
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] AnonHxy commented on pull request #15139: [fix][broker]Fix create topic fail deadlock
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #15139:
URL: https://github.com/apache/pulsar/pull/15139#issuecomment-1098779653
> I've see many exception branch ends with a line: `pulsar.getExecutor().execute(() -> topics.remove(topic, topicFuture));`, that's weird by the way.
>
> It seems just a cleanup operation and it could be handled in `only one place`, such as below:
>
> ```diff
> Index: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
> IDEA additional info:
> Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
> <+>UTF-8
> ===================================================================
> diff --git a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
> --- a/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java (revision 5433d2ea03ff97a688e9c493e54282e1df07d66d)
> +++ b/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java (date 1649909372658)
> @@ -977,12 +977,13 @@
> }
> }
> final boolean isPersistentTopic = TopicName.get(topic).getDomain().equals(TopicDomain.persistent);
> + CompletableFuture<Optional<Topic>> loadTopicFuture;
> if (isPersistentTopic) {
> - return topics.computeIfAbsent(topic, (topicName) -> {
> + loadTopicFuture = topics.computeIfAbsent(topic, (topicName) -> {
> return this.loadOrCreatePersistentTopic(topicName, createIfMissing, properties);
> });
> } else {
> - return topics.computeIfAbsent(topic, (name) -> {
> + loadTopicFuture = topics.computeIfAbsent(topic, (name) -> {
> final TopicName topicName = TopicName.get(name);
> if (topicName.isPartitioned()) {
> final TopicName partitionedTopicName = TopicName.get(topicName.getPartitionedTopicName());
> @@ -999,6 +1000,12 @@
> }
> });
> }
> + return loadTopicFuture.exceptionally(ignore -> {
> + // wo don't need remove entry in another thread any more
> + // cause we are not in the computeIfAbsent block
> + topics.remove(topic, loadTopicFuture);
> + return null;
> + });
> } catch (IllegalArgumentException e) {
> log.warn("[{}] Illegalargument exception when loading topic", topic, e);
> return FutureUtil.failedFuture(e);
> ```
Good point! I think we can change this in a new PR @Shawyeok
--
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] AnonHxy commented on pull request #15139: [fix][broker]Fix create topic fail deadlock
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #15139:
URL: https://github.com/apache/pulsar/pull/15139#issuecomment-1098655253
>
Make sense. The latest commit also fix `ConcurrentOpenHashMap`, PTAL @shibd @gaozhangmin @HQebupt @Technoboy-
--
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] shibd commented on pull request #15139: [fix][broker]Fix create topic fail deadlock
Posted by GitBox <gi...@apache.org>.
shibd commented on PR #15139:
URL: https://github.com/apache/pulsar/pull/15139#issuecomment-1098237643
> If topics.remove throw exceptions, the topicFuture will not be done. So we need check the existed code like this and also avoid in the future. So in my opinion， we'd better keep it the way it is.@shibd
I think if topicFuture not be done, It can reflect that the code has bugs. This is more friendly than a deadlock.
--
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] AnonHxy commented on pull request #15139: [fix][broker]Fix create topic fail deadlock
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #15139:
URL: https://github.com/apache/pulsar/pull/15139#issuecomment-1125703667
https://github.com/apache/pulsar/pull/15570 has fix it and merged, I will close this PR
--
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] shibd commented on pull request #15139: [fix][broker]Fix create topic fail deadlock
Posted by GitBox <gi...@apache.org>.
shibd commented on PR #15139:
URL: https://github.com/apache/pulsar/pull/15139#issuecomment-1097939550
In order to avoid the same problem, do we need to improve `ConcurrentOpenHashMap`?
In `ConcurrentHashMap`, The same operation throws an exception, which makes it easier to spot the problem.
For example below:
```
Map<String, String> map = new ConcurrentHashMap<>();
map.computeIfAbsent("test", (key) -> {
map.remove("test");
return "ok";
});
```
It will throw `java.lang.IllegalStateException: Recursive update`
--
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] HQebupt commented on a diff in pull request #15139: [fix][broker]Fix create topic fail deadlock
Posted by GitBox <gi...@apache.org>.
HQebupt commented on code in PR #15139:
URL: https://github.com/apache/pulsar/pull/15139#discussion_r849026303
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -1443,10 +1443,10 @@ public void openLedgerComplete(ManagedLedger ledger, Object ctx) {
.exceptionally((ex) -> {
log.warn("Replication or dedup check failed."
+ " Removing topic from topics list {}, {}", topic, ex);
- persistentTopic.stopReplProducers().whenComplete((v, exception) -> {
Review Comment:
Does the line 1435 need to do the same thing? `persistentTopic.stopReplProducers().whenComplete((v, exception)`
--
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] AnonHxy commented on pull request #15139: [fix][broker]Fix create topic fail deadlock
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #15139:
URL: https://github.com/apache/pulsar/pull/15139#issuecomment-1106624153
> in theory we should follow the semantics of computeIfAbsent, that is...if the operation fails we should return null, and not try to remove something that is not in the map
>
> what about this:
>
> ```
> if (topics.contains(topic)) {
> topics.remove(topic, topicFuture);
> }
> ```
>
> so, if we are in the case of "computeIfAbsent" when the topicFuture has not been added to the map, we do nothing. if we are in other code paths, in which the topic is already in the map, we do the remove operation
Make sense. But there are some other cases like this. It is better to fix this case in the same way, I think ...
--
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] AnonHxy commented on pull request #15139: [fix][broker]Fix create topic fail deadlock
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #15139:
URL: https://github.com/apache/pulsar/pull/15139#issuecomment-1118913741
/pulsarbot run-failure-checks
--
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