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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
