[fix] [broker] Remove the repl cursor if the creation fails and the replication is turned off by poorbarcode · Pull Request #20025 · apache/pulsar

poorbarcode · 2023-04-06T04:56:54Z

Fixes #20010

Motivation

The Cursorrepl is automatically created when replication is turned on and it will be deleted when replication is turned off. But there has a bug: if the Topic is not online when you turn off replication, the cursor cannot be deleted, and #19972 fixes this bug.

But #19972 cannot solve the following scenario that cause the flaky test #20010:

a replication task starting
get clusters( responses [local_cluster, remote_cluster])
create cursor repl
at this moment, the remote_cluster is removed, and the remote cluster is down now.
then the task start replicator will fail, but the cursor still there

Modifications

Differing with #19972, do remove the repl cursor when the task start replicator is failed.

Documentation

doc
doc-required
doc-not-needed
doc-complete

Matching PR in forked repository

PR in forked repository:

[fix] [broker] Ignore and remove the replicator cursor when the remot… poorbarcode/pulsar#84

…e cluster is absent

BewareMyPower · 2023-04-06T06:50:46Z

...ar-broker/src/test/java/org/apache/pulsar/broker/service/persistent/PersistentTopicTest.java


        if (topicLevelPolicy) {
-            admin.topics().setReplicationClusters(topicName, Collections.singletonList(remoteCluster));
+            admin.topics().setReplicationClusters(topicName, Arrays.asList(remoteCluster, "test"));


Why did you add another cluster "test" to this test? Even without the change of PersistentTopic, this test could still pass.

If we disabled replication, the clusters will contain one cluster test. See: https://github.com/apache/pulsar/blob/master/pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BrokerTestBase.java#L55-L56

If we enabled replication, the clusters will contain two clusters: local cluster and remote cluster

If the cluster is not set correctly, it can mislead others: clusters is an empty collection when Replication is disabled.

I think it's another topic about the difference determined by whether the replication is enabled. It does not affect this test.

I think it's another topic about the difference determined by whether the replication is enabled. It does not affect this test.

Correct.

So, it's better to revert this unrelated change.

So @poorbarcode Is there any other reason to add the local cluster here? It might be misleading that adding this change could make test better.

reverted thse changes

BewareMyPower · 2023-04-06T07:25:30Z

It's still flaky.

java.util.concurrent.ExecutionException: java.lang.RuntimeException: org.apache.pulsar.metadata.api.MetadataStoreException$NotFoundException: remote

	at java.base/java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:396)
	at java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2096)
	at org.apache.pulsar.broker.service.persistent.PersistentTopicTest.testCreateTopicWithZombieReplicatorCursor(PersistentTopicTest.java:587)

BewareMyPower · 2023-04-06T10:37:31Z

Could you also change the title? The PR title is duplicated with #19972.

BewareMyPower · 2023-04-06T11:23:02Z

...ar-broker/src/test/java/org/apache/pulsar/broker/service/persistent/PersistentTopicTest.java

+        Awaitility.await().atMost(10, TimeUnit.SECONDS)
+                .until(() -> {
+                    topic.initialize().get(3, TimeUnit.SECONDS);


It's meaningless because topic.initialize might throw an exception and it won't be retried.

java.util.concurrent.ExecutionException: java.lang.RuntimeException: org.apache.pulsar.metadata.api.MetadataStoreException$NotFoundException: remote at java.base/java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:396) at java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2096) at org.apache.pulsar.broker.service.persistent.PersistentTopicTest.lambda$testCreateTopicWithZombieReplicatorCursor$10(PersistentTopicTest.java:602) at org.awaitility.core.CallableCondition$ConditionEvaluationWrapper.eval(CallableCondition.java:99)

perhaps .ignoreExceptionsInstanceOf(MetadataStoreException.NotFoundException.class) could help?

BTW, the root cause is not here. I added a try-catch block to swallow the exception, the test could still fail.

try { topic.initialize().get(3, TimeUnit.SECONDS); } catch (ExecutionException e) { log.warn("Failed to initialize: {}", e.getCause().getMessage()); }

BTW, the root cause is not here. I added a try-catch block to swallow the exception, the test could still fail.

The root cause is like this:

a replication task starting

failed to get cluster meta from ZK

check replication policies

Since the policy is updated asynchronously, the cache has not been updated.

get clusters( responses [local_cluster, remote_cluster])

Since check replication successes, will not delete the cursor. bingo

We don't need to guarantee that the cursor will be deleted the first time because the client will try to reload the topic. We just need to guarantee that the cursor will be deleted eventually. So I did this change:

Awaitility.await().atMost(10, TimeUnit.SECONDS) .until(() -> { topic.initialize().get(3, TimeUnit.SECONDS); return !topic.getManagedLedger().getCursors().iterator().hasNext(); });

topic.initialize().get(3, TimeUnit.SECONDS); just mock the load operation by client, and verify that the cursor will be deleted eventually.

I've run this test 4*200 times with no errors

just mock the load operation by client,

Actually, the operation failed at topic.initialize in my local env. An exception will be thrown and it won't retry.

See step 4 above, it goes to the logic branch else, and the error is thrown.

https://github.com/apache/pulsar/pull/20025/files#diff-5edf14cc6f25857d0cfdd26b2d3b3141230ecfb0dfa95aebf7583fd76ede4c4bR1748-R1751

I have change the test to Awaitility.await().atMost(10, TimeUnit.SECONDS).ignoreExceptions().untilAsserted(, it will be fixed

codecov-commenter · 2023-04-06T12:20:15Z

Codecov Report

Merging #20025 (903466c) into master (8c50a6c) will increase coverage by 39.64%.
The diff coverage is 76.66%.

@@              Coverage Diff              @@
##             master   #20025       +/-   ##
=============================================
+ Coverage     33.17%   72.81%   +39.64%     
- Complexity    12158    31601    +19443     
=============================================
  Files          1499     1860      +361     
  Lines        113832   137406    +23574     
  Branches      12368    15131     +2763     
=============================================
+ Hits          37769   100059    +62290     
+ Misses        71143    29381    -41762     
- Partials       4920     7966     +3046

Flag	Coverage Δ
inttests	`24.41% <43.33%> (?)`
systests	`24.87% <0.00%> (?)`
unittests	`72.11% <76.66%> (+38.93%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...sar/broker/service/persistent/PersistentTopic.java	`79.49% <76.66%> (+29.87%)`	⬆️

... and 1530 files with indirect coverage changes

lhotari · 2023-04-06T12:28:21Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java

+                            replicationStartFuture.complete(null);
+                            return removeReplicator(remoteCluster).whenComplete((ignore2, remoteCursorEx) -> {


why is the replicationStartFuture completed before removing the replicator?

Already fixed. Thanks

lhotari · 2023-04-06T12:29:48Z

...ar-broker/src/test/java/org/apache/pulsar/broker/service/persistent/PersistentTopicTest.java

+                try {
+                    admin.topics().setReplicationClusters(topicName, Arrays.asList(remoteCluster, "test"));
+                    return true;
+                } catch (Exception ex) {
+                    return false;
+                }


You could use Awaitility.await().ignoreExceptions().untilAsserted( instead of the try / ... / return true / catch Exception ... return false solution.

Good idea. Thanks, already change to Awaitility.await().ignoreExceptions().untilAsserted

BewareMyPower · 2023-04-06T12:49:18Z

I've fixed it locally. I will open another PR to test.

BewareMyPower · 2023-04-06T13:45:11Z

@poorbarcode Could you also change the title? The PR title is duplicated with #19972.

poorbarcode · 2023-04-06T13:53:29Z

Could you also change the title? The PR title is duplicated with #19972.

Done

BewareMyPower · 2023-04-06T13:57:24Z

Here is a failure from my local env: failure.log

I see many Starting replicator logs, we should also check the remote clusters policy before calling startReplicator, which creates a replication cursor and could be called by:

onPoliciesUpdate
  checkReplicationAndRetryOnFailure
    checkReplication

BewareMyPower · 2023-04-06T15:59:01Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java

+            }).whenComplete((ignore, ex) -> {
+                if (ex == null){
+                    replicationStartFuture.complete(null);
+                } else {


It's better to print the ex warning here.

BewareMyPower · 2023-04-07T07:28:13Z

After discussing with @poorbarcode offline, let's move to #20037 to fix the flaky test.

michaeljmarshall · 2023-06-27T21:56:00Z

As discussed on the mailing list https://lists.apache.org/thread/w4jzk27qhtosgsz7l9bmhf1t7o9mxjhp, there is no plan to release 2.9.6, so I am going to remove the release/2.9.6 label

[fix] [broker] Ignore and remove the replicator cursor when the remot…

9dd077e

…e cluster is absent

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Apr 6, 2023

poorbarcode mentioned this pull request Apr 6, 2023

Flaky-test: PersistentTopicTest.testCreateTopicWithZombieReplicatorCursor #20010

Closed

2 tasks

poorbarcode requested review from BewareMyPower and lhotari April 6, 2023 04:58

poorbarcode self-assigned this Apr 6, 2023

poorbarcode added release/2.9.5 release/2.9.6 release/2.10.5 release/2.11.2 type/bug The PR fixed a bug or issue reported a bug area/geo-replication and removed release/2.9.5 labels Apr 6, 2023

BewareMyPower reviewed Apr 6, 2023

View reviewed changes

Technoboy- added the ready-to-test label Apr 6, 2023

Technoboy- added this to the 3.0.0 milestone Apr 6, 2023

fix flaky test

903466c

BewareMyPower reviewed Apr 6, 2023

View reviewed changes

lhotari reviewed Apr 6, 2023

View reviewed changes

poorbarcode added 2 commits April 6, 2023 21:07

address comments

1ce4fb9

address comment

20f8a39

poorbarcode requested review from BewareMyPower and lhotari April 6, 2023 13:19

code format

5e15522

poorbarcode changed the title ~~[fix] [broker] Ignore and remove the replicator cursor when the remote cluster is absent~~ [fix] [broker] Remove the repl cursor if the creation fails and the replication is turned off Apr 6, 2023

add logs

593e66c

BewareMyPower reviewed Apr 6, 2023

View reviewed changes

poorbarcode added 2 commits April 7, 2023 00:02

revert the changes which not effected of the flaky test

dd5aafa

revert the changes which not effected of the flaky test

395f40c

poorbarcode closed this Apr 7, 2023

michaeljmarshall removed the release/2.9.6 label Jun 27, 2023

		replicationStartFuture.complete(null);
		return removeReplicator(remoteCluster).whenComplete((ignore2, remoteCursorEx) -> {

Conversation

poorbarcode commented Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Documentation

Matching PR in forked repository

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poorbarcode Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BewareMyPower commented Apr 6, 2023

Uh oh!

BewareMyPower commented Apr 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poorbarcode Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poorbarcode Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Apr 6, 2023

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhotari Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BewareMyPower commented Apr 6, 2023

Uh oh!

BewareMyPower commented Apr 6, 2023

Uh oh!

poorbarcode commented Apr 6, 2023

Uh oh!

BewareMyPower commented Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BewareMyPower commented Apr 7, 2023

Uh oh!

michaeljmarshall commented Jun 27, 2023

Uh oh!

Reviewers

Assignees

poorbarcode commented Apr 6, 2023 •

edited

Loading

poorbarcode Apr 6, 2023 •

edited

Loading

poorbarcode Apr 6, 2023 •

edited

Loading

poorbarcode Apr 6, 2023 •

edited

Loading

lhotari Apr 6, 2023 •

edited

Loading

BewareMyPower commented Apr 6, 2023 •

edited

Loading