[fix][broker] Ignore and remove the replicator cursor when the remote cluster is absent #19972

BewareMyPower · 2023-03-30T17:12:17Z

Motivation

Sometimes when a remote cluster is deleted, the replication cursor might still exist for some topics. In this case, creating producers or consumers on these topics will fail.

Here is a log observed in a production environment:

WARN org.apache.pulsar.broker.service.BrokerService - Replication or
dedup check failed. Removing topic from topics list
persistent://public/__kafka/__consumer_offsets-partition-40,
java.util.concurrent.CompletionException: java.lang.RuntimeException:
org.apache.pulsar.metadata.api.MetadataStoreException$NotFoundException:
kop

If it happened, unloading the topic or restarting the broker could not help. We have to remove the cursor manually.

Modifications

In addReplicationCluster, before getting the replication client, check the namespace policy and topic policy first. If the remote cluster does not exist, skip adding the replication client and remove the cursor.

Verifications

PersistentTopicTest#testCreateTopicWithZombieReplicatorCursor is added to verify PersistentTopic#initialize will succeed and the zombie replicator cursor will be removed.

Documentation

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

Matching PR in forked repository

PR in forked repository: BewareMyPower#23

github-actions · 2023-03-30T17:12:53Z

@BewareMyPower Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

poorbarcode

ShadowReplicator hits this issue, too. Could you please also fix it?

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

BewareMyPower · 2023-03-31T04:40:34Z

ShadowReplicator hits this issue, too. Could you please also fix it?

I think it's another issue. You can open another PR for it.

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

… cluster is absent ### Motivation Sometimes when a remote cluster is deleted, the replication cursor might still exist for some topics. In this case, creating producers or consumers on these topics will fail. Here is a log observed in a production environment: > WARN org.apache.pulsar.broker.service.BrokerService - Replication or > dedup check failed. Removing topic from topics list > persistent://public/__kafka/__consumer_offsets-partition-40, > java.util.concurrent.CompletionException: java.lang.RuntimeException: > org.apache.pulsar.metadata.api.MetadataStoreException$NotFoundException: > kop If it happened, unloading the topic or restarting the broker could not help. We have to remove the cursor manually. ### Modificatons In `addReplicationCluster`, before getting the replication client, check the namespace policy and topic policy first. If the remote cluster does not exist, skip adding the replication client and remove the cursor. ### Verifications `PersistentTopicTest#testCreateTopicWithZombieReplicatorCursor` is added to verify `PersistentTopic#initialize` will succeed and the zombie replicator cursor will be removed.

BewareMyPower · 2023-03-31T09:33:29Z

Hi, @poorbarcode @315157973 Now I checked the namespace and topic policies to determine whether to delete the cursor.

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

codecov-commenter · 2023-04-03T10:46:15Z

Codecov Report

Merging #19972 (335b4d6) into master (68c10ee) will decrease coverage by 0.05%.
The diff coverage is 94.44%.

@@             Coverage Diff              @@
##             master   #19972      +/-   ##
============================================
- Coverage     72.89%   72.85%   -0.05%     
+ Complexity    31619    31613       -6     
============================================
  Files          1861     1861              
  Lines        137356   137375      +19     
  Branches      15117    15120       +3     
============================================
- Hits         100131   100089      -42     
- Misses        29271    29329      +58     
- Partials       7954     7957       +3

Flag	Coverage Δ
inttests	`24.28% <50.00%> (-0.15%)`	⬇️
systests	`25.00% <0.00%> (-0.04%)`	⬇️
unittests	`72.15% <94.44%> (-0.03%)`	⬇️

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

Impacted Files	Coverage Δ
...sar/broker/service/persistent/PersistentTopic.java	`80.15% <94.44%> (+0.24%)`	⬆️

... and 77 files with indirect coverage changes

315157973 · 2023-04-03T11:20:08Z

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

+                .thenCompose(__ -> checkReplicationCluster(remoteCluster))
+                .thenCompose(clusterExists -> {
+                    if (!clusterExists) {
+                        log.warn("Remove the replicator because the cluster '{}' does not exist", remoteCluster);
+                        return removeReplicator(remoteCluster).thenApply(__ -> null);


Why don't we clean up when the Policy is removed?

Sorry I missed this comment when I merged. And yes, we should do that as well. WDYT? @poorbarcode

@315157973

Why don't we clean up when the Policy is removed?

When the policy is removed, the cursor will not be clean If this topic is not online. see

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

Line 1589 in d1fc732

futures.add(removeShadowReplicator(shadowTopic));

… cluster is absent (#19972) (cherry picked from commit d1fc732)

lhotari · 2023-04-04T20:49:40Z

This PR introduced a new flaky test, reported as #20010 . @BewareMyPower do you have a chance to take a look? thanks

BewareMyPower · 2023-04-05T14:19:53Z

Okay, I assigned that issue to me first.

Technoboy- · 2023-04-10T01:55:10Z

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

-        replicators.get(remoteCluster).disconnect().thenRun(() -> {
-
+        Optional.ofNullable(replicators.get(remoteCluster)).map(Replicator::disconnect)
+                .orElse(CompletableFuture.completedFuture(null)).thenRun(() -> {


orElseGet is better

I think not. See

public static <U> CompletableFuture<U> completedFuture(U value) { return new CompletableFuture<U>((value == null) ? NIL : value); }

so completedFuture(null) won't create a new instance.

… cluster is absent (#19972)

… cluster is absent (apache#19972) (cherry picked from commit d1fc732) (cherry picked from commit b93176c)

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

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

…ting logic for better handle the checkpoint. (apache#19972) * Change the initial start cursor and stop cursor to better handle the consuming behaviors. * Create the initial subscription instead seek every time. This should fix the wrong position setting. * Fix the wrong stop cursor, make sure it stops at the correct space * Drop Consumer.seek() for apache#16171

BewareMyPower added type/bug The PR fixed a bug or issue reported a bug area/broker area/geo-replication release/2.10.5 release/2.9.6 release/2.11.2 labels Mar 30, 2023

BewareMyPower added this to the 3.0.0 milestone Mar 30, 2023

BewareMyPower requested review from eolivelli, 315157973, codelipenghui, Demogorgon314, poorbarcode and coderzc March 30, 2023 17:12

BewareMyPower self-assigned this Mar 30, 2023

github-actions bot added the doc-label-missing label Mar 30, 2023

github-actions bot added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Mar 30, 2023

poorbarcode reviewed Mar 31, 2023

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java Outdated Show resolved Hide resolved

315157973 reviewed Mar 31, 2023

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java Outdated Show resolved Hide resolved

BewareMyPower marked this pull request as draft March 31, 2023 07:50

BewareMyPower force-pushed the bewaremypower/replicator-zombie-cursors branch from 1469b85 to 8aeb37d Compare March 31, 2023 09:30

BewareMyPower marked this pull request as ready for review March 31, 2023 09:31

poorbarcode approved these changes Apr 1, 2023

View reviewed changes

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

Add warn logs

335b4d6

315157973 approved these changes Apr 3, 2023

View reviewed changes

315157973 reviewed Apr 3, 2023

View reviewed changes

BewareMyPower merged commit d1fc732 into apache:master Apr 4, 2023

BewareMyPower added a commit that referenced this pull request Apr 4, 2023

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

b93176c

… cluster is absent (#19972) (cherry picked from commit d1fc732)

BewareMyPower added the cherry-picked/branch-2.10 label Apr 4, 2023

lhotari mentioned this pull request Apr 5, 2023

Flaky-test: PersistentTopicTest.testCreateTopicWithZombieReplicatorCursor #20010

Closed

2 tasks

liangyepianzhou added release/2.10.4 and removed release/2.10.5 labels Apr 6, 2023

poorbarcode mentioned this pull request Apr 6, 2023

[fix] [broker] Remove the repl cursor if the creation fails and the replication is turned off #20025

Closed

4 tasks

Technoboy- reviewed Apr 10, 2023

View reviewed changes

Technoboy- pushed a commit that referenced this pull request May 9, 2023

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

9948f89

… cluster is absent (#19972)

Technoboy- added the cherry-picked/branch-2.11 label May 9, 2023

nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request May 11, 2023

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

77190ce

… cluster is absent (apache#19972) (cherry picked from commit d1fc732) (cherry picked from commit b93176c)

michaeljmarshall removed the release/2.9.6 label Jun 27, 2023

poorbarcode mentioned this pull request Jun 11, 2024

[fix] [broker] Messages lost on the remote cluster when using topic level replication #22890

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix][broker] Ignore and remove the replicator cursor when the remote cluster is absent #19972

[fix][broker] Ignore and remove the replicator cursor when the remote cluster is absent #19972

BewareMyPower commented Mar 30, 2023 •

edited

Loading

github-actions bot commented Mar 30, 2023

poorbarcode left a comment

BewareMyPower commented Mar 31, 2023

BewareMyPower commented Mar 31, 2023

codecov-commenter commented Apr 3, 2023

315157973 Apr 3, 2023

BewareMyPower Apr 4, 2023

poorbarcode Apr 4, 2023

lhotari commented Apr 4, 2023

BewareMyPower commented Apr 5, 2023

Technoboy- Apr 10, 2023

BewareMyPower Apr 10, 2023

michaeljmarshall commented Jun 27, 2023

[fix][broker] Ignore and remove the replicator cursor when the remote cluster is absent #19972

[fix][broker] Ignore and remove the replicator cursor when the remote cluster is absent #19972

Conversation

BewareMyPower commented Mar 30, 2023 • edited Loading

Motivation

Modifications

Verifications

Documentation

Matching PR in forked repository

github-actions bot commented Mar 30, 2023

poorbarcode left a comment

Choose a reason for hiding this comment

BewareMyPower commented Mar 31, 2023

BewareMyPower commented Mar 31, 2023

codecov-commenter commented Apr 3, 2023

Codecov Report

315157973 Apr 3, 2023

Choose a reason for hiding this comment

BewareMyPower Apr 4, 2023

Choose a reason for hiding this comment

poorbarcode Apr 4, 2023

Choose a reason for hiding this comment

lhotari commented Apr 4, 2023

BewareMyPower commented Apr 5, 2023

Technoboy- Apr 10, 2023

Choose a reason for hiding this comment

BewareMyPower Apr 10, 2023

Choose a reason for hiding this comment

michaeljmarshall commented Jun 27, 2023

BewareMyPower commented Mar 30, 2023 •

edited

Loading