Fix call sync method in an async callback when enabling geo replicator. #12590

codelipenghui · 2021-11-02T16:12:04Z

After enabled geo-replication, we are not able to produce messages to a partitioned non-persistent topic.

Reproduce step:

Start a geo-replication instance with 2 clusters, I have 3 brokers for each cluster
Create a non-persistent partitioned topic such as 10 partitions
Use pulsar-perf to publish messages to the partitioned topic
Verify if the message produce throughput is 0, such as bin/pulsar-perf produce -s 2048 -r 100 -bm 1 non-persistent://public/default/test

The root cause is there are 2 places calling a sync method in the async method callback.
So the fix is:

Async the validatePartitionedTopicAsync method
Avoid call get cluster sync method when getting the replication client

Integration tests added.

Documentation

Check the box below and label this PR (if you have committer privilege).

Need to update docs?

doc-required

(If you need help on updating docs, create a doc issue)
no-need-doc

Bug fixes
doc

(If this PR contains doc changes)

After enabled geo-replication, we are not able to produce messages to a partitioned non-persistent topic. Reproduce step: 1. Start a geo-replication instance with 2 clusters, I have 3 brokers for each cluster 2. Create a non-persistent partitioned topic such as 10 partitions 3. Use pulsar-perf to publish messages to the partitioned topic 4. Verify if the message produce throughput is 0, such as `bin/pulsar-perf produce -s 2048 -r 100 -bm 1 non-persistent://public/default/test` The root cause is there are 2 places calling a sync method in the async method callback. So the fix is: 1. Async the `validatePartitionedTopicAsync` method 2. Avoid call get cluster sync method when getting the replication client Integration tests added.

github-actions · 2021-11-02T16:12:34Z

@codelipenghui:Thanks for providing doc info!

github-actions · 2021-11-02T16:12:47Z

@codelipenghui:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

eolivelli

LGTM

if GH actions has not enough resources to boot multiple clusters we can split this patch into two parts and commit only the fix to branch-2.9

…en creating partitioned-topic, this will cause dead thread problem.

eolivelli · 2021-11-05T08:00:29Z

@codelipenghui do you need some help here ?
this is the unblocker for the 2.9.0 release

codelipenghui · 2021-11-05T10:20:58Z

@eolivelli Thanks, @congbobo184 is helping on the test.

gaoran10

LGTM

codelipenghui · 2021-11-05T10:56:47Z

@merlimat Could you please help review the PR?

eolivelli

LGTM
@codelipenghui
can you please take a look to my latest comments ?

eolivelli · 2021-11-05T10:53:52Z

pulsar-broker-common/src/main/java/org/apache/pulsar/broker/resources/NamespaceResources.java

@@ -122,7 +118,7 @@ public void setPolicies(NamespaceName ns, Function<Policies, Policies> function)
    }

    public static boolean pathIsFromNamespace(String path) {
-        return path.startsWith(BASE_POLICIES_PATH);
+        return path.startsWith(BASE_POLICIES_PATH) && path.substring(BASE_POLICIES_PATH.length() + 1).contains("/");


can you explain better this line ?

We may receive a notification from the metastore /admin/policies/public, not a complete namespace path

I see.
works for me, probably adding a comment will help future readers of this code

eolivelli · 2021-11-05T10:54:49Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java

+                                                topicName.getPartitionedTopicName(), numPartitions, true);
+                            })
+                            .exceptionally(throwable -> {
+                                log.error("Failed to create partition topic in cluster {}.", cluster, throwable);


should we fail the operation ?

eolivelli · 2021-11-05T10:55:50Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java

+                log.warn("Replication check failed. Removing topic from topics list {}, {}", topic, ex.getCause());
+                nonPersistentTopic.stopReplProducers().whenComplete((v, exception) -> {
+                    pulsar.getExecutor().execute(() -> topics.remove(topic, topicFuture));
+                    topicFuture.completeExceptionally(ex);


should we run this after the execution of "topics.remove" ?

No effect before and after

good, just checking

@test

* up/master: (55 commits) [broker] remove useless method "PersistentTopic#getPersistentTopic" (apache#12655) [Python Schema] Python schema support custom Avro configurations for Enum type (apache#12642) Allow to configure different implementations for Pulsar functions state store (apache#12646) Remove replicator global test from the quarantine group (apache#12648) [Java Client] Remove invalid call to Thread.currentThread().interrupt(); (apache#12652) k8s runtime: force deletion to avoid hung function worker during connector restart (apache#12504) [Broker] Optimize exception information for schemas (apache#12647) Close Zk database on unit tests (apache#12649) Fix call sync method in an async callback when enabling geo replicator. (apache#12590) [pulsar-broker] Add git branch information for PulsarVersion (apache#12541) PulsarAdmin: Fix last exit code storage (apache#12581) Add @test annotation to test methods (apache#12640) Upgrade debezium to 1.7.1 (apache#12644) [ML] Avoid passing OpAddEntry across a thread boundary in asyncAddEntry (apache#12606) [Functions] Prevent NPE while stopping a non started Pulsar LogAppender (apache#12643) Update io-debezium-source.md (apache#12638) Add missing cmds on pulsar-admin document page (apache#12634) Clean up the metadata of the non-persistent partitioned topics. (apache#12550) modify check waitingForPingResponse with volatile (apache#12615) [pulsar-admin] Check backlog quota policy for namespace (apache#12512) ...

@test

* up/master: (55 commits) [broker] remove useless method "PersistentTopic#getPersistentTopic" (apache#12655) [Python Schema] Python schema support custom Avro configurations for Enum type (apache#12642) Allow to configure different implementations for Pulsar functions state store (apache#12646) Remove replicator global test from the quarantine group (apache#12648) [Java Client] Remove invalid call to Thread.currentThread().interrupt(); (apache#12652) k8s runtime: force deletion to avoid hung function worker during connector restart (apache#12504) [Broker] Optimize exception information for schemas (apache#12647) Close Zk database on unit tests (apache#12649) Fix call sync method in an async callback when enabling geo replicator. (apache#12590) [pulsar-broker] Add git branch information for PulsarVersion (apache#12541) PulsarAdmin: Fix last exit code storage (apache#12581) Add @test annotation to test methods (apache#12640) Upgrade debezium to 1.7.1 (apache#12644) [ML] Avoid passing OpAddEntry across a thread boundary in asyncAddEntry (apache#12606) [Functions] Prevent NPE while stopping a non started Pulsar LogAppender (apache#12643) Update io-debezium-source.md (apache#12638) Add missing cmds on pulsar-admin document page (apache#12634) Clean up the metadata of the non-persistent partitioned topics. (apache#12550) modify check waitingForPingResponse with volatile (apache#12615) [pulsar-admin] Check backlog quota policy for namespace (apache#12512) ...

…r. (#12590) After enabled geo-replication, we are not able to produce messages to a partitoned non-persistent topic. Reproduce step: 1. Start a geo-replication instance with 2 clusters, I have 3 brokers for each cluster 2. Create a non-persistent partitioned topic such as 10 partitions 3. Use pulsar-perf to publish messages to the partitioned topic 4. Verify if the message produce throughput is 0, such as `bin/pulsar-perf produce -s 2048 -r 100 -bm 1 non-persistent://public/default/test` The root cause is there are 2 places calling a sync method in the async method callback. So the fix is: 1. Async the `validatePartitionedTopicAsync` method 2. Avoid call get cluster sync method when getting the replication client Integration tests added. (cherry picked from commit a2b7cae)

…r. (apache#12590) After enabled geo-replication, we are not able to produce messages to a partitioned non-persistent topic. Reproduce step: 1. Start a geo-replication instance with 2 clusters, I have 3 brokers for each cluster 2. Create a non-persistent partitioned topic such as 10 partitions 3. Use pulsar-perf to publish messages to the partitioned topic 4. Verify if the message produce throughput is 0, such as `bin/pulsar-perf produce -s 2048 -r 100 -bm 1 non-persistent://public/default/test` The root cause is there are 2 places calling a sync method in the async method callback. So the fix is: 1. Async the `validatePartitionedTopicAsync` method 2. Avoid call get cluster sync method when getting the replication client Integration tests added.

codelipenghui self-assigned this Nov 2, 2021

codelipenghui added this to the 2.9.0 milestone Nov 2, 2021

codelipenghui added the release/blocker Indicate the PR or issue that should block the release until it gets resolved label Nov 2, 2021

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Nov 2, 2021

codelipenghui requested review from merlimat, eolivelli, rdhabalia, 315157973, congbobo184, gaoran10, hangc0276 and zymap November 2, 2021 16:12

github-actions bot added the doc-label-missing label Nov 2, 2021

codelipenghui added type/bug The PR fixed a bug or issue reported a bug and removed doc-label-missing labels Nov 2, 2021

Fix tests.

aa6130d

eolivelli approved these changes Nov 3, 2021

View reviewed changes

codelipenghui and others added 5 commits November 4, 2021 13:06

Fix tests.

abd5360

Fix tests.

53ba4b4

Fix tests.

b9b13e6

Fix tests.

7836fd3

Fix getting cluster data synchronously in an asynchronously method wh…

803ca3e

…en creating partitioned-topic, this will cause dead thread problem.

codelipenghui added the release/2.9.0 label Nov 4, 2021

fix non-persistent-topc create logic

b8f5b26

gaoran10 approved these changes Nov 5, 2021

View reviewed changes

eolivelli approved these changes Nov 5, 2021

View reviewed changes

Merge branch 'master' into penghui/fix-replicator-async

43d0e96

merlimat approved these changes Nov 6, 2021

View reviewed changes

Fix code style and change some annotation

baaf6b0

codelipenghui merged commit a2b7cae into apache:master Nov 6, 2021

codelipenghui deleted the penghui/fix-replicator-async branch November 6, 2021 14:06

Technoboy- mentioned this pull request Nov 10, 2021

Remove catching NamingException. #12725

Merged

Technoboy- mentioned this pull request Feb 23, 2022

[PIP-149] Making the REST Admin API fully async #14365

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix call sync method in an async callback when enabling geo replicator. #12590

Fix call sync method in an async callback when enabling geo replicator. #12590

codelipenghui commented Nov 2, 2021

github-actions bot commented Nov 2, 2021

github-actions bot commented Nov 2, 2021

eolivelli left a comment

eolivelli commented Nov 5, 2021

codelipenghui commented Nov 5, 2021

gaoran10 left a comment

codelipenghui commented Nov 5, 2021

eolivelli left a comment

eolivelli Nov 5, 2021

codelipenghui Nov 5, 2021

eolivelli Nov 5, 2021

eolivelli Nov 5, 2021

eolivelli Nov 5, 2021

congbobo184 Nov 5, 2021

eolivelli Nov 5, 2021

Fix call sync method in an async callback when enabling geo replicator. #12590

Fix call sync method in an async callback when enabling geo replicator. #12590

Conversation

codelipenghui commented Nov 2, 2021

Documentation

github-actions bot commented Nov 2, 2021

github-actions bot commented Nov 2, 2021

eolivelli left a comment

Choose a reason for hiding this comment

eolivelli commented Nov 5, 2021

codelipenghui commented Nov 5, 2021

gaoran10 left a comment

Choose a reason for hiding this comment

codelipenghui commented Nov 5, 2021

eolivelli left a comment

Choose a reason for hiding this comment

eolivelli Nov 5, 2021

Choose a reason for hiding this comment

codelipenghui Nov 5, 2021

Choose a reason for hiding this comment

eolivelli Nov 5, 2021

Choose a reason for hiding this comment

eolivelli Nov 5, 2021

Choose a reason for hiding this comment

eolivelli Nov 5, 2021

Choose a reason for hiding this comment

congbobo184 Nov 5, 2021

Choose a reason for hiding this comment

eolivelli Nov 5, 2021

Choose a reason for hiding this comment