[fix][broker]fail to update partition meta of topic due to ConflictException: subscription already exists for topic #17251

HQebupt · 2022-08-24T04:04:46Z

Motivation

#10374 wants to handle the existing subscriptions when updating partition with force command parameter. However, it could not update the partition metadata if the Subscription already exists for topic exception comes. Because it miss updating when handling such exception.

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java

Lines 4360 to 4392 in 3160edc

    
           private CompletableFuture<Void> updatePartitionedTopic(TopicName topicName, int numPartitions, boolean force) { 
        
               CompletableFuture<Void> result = new CompletableFuture<>(); 
        
               createSubscriptions(topicName, numPartitions).thenCompose(__ -> { 
        
                   CompletableFuture<Void> future = namespaceResources().getPartitionedTopicResources() 
        
                           .updatePartitionedTopicAsync(topicName, p -> new PartitionedTopicMetadata(numPartitions)); 
        
                   future.exceptionally(ex -> { 
        
                       // If the update operation fails, clean up the partitions that were created 
        
                       getPartitionedTopicMetadataAsync(topicName, false, false).thenAccept(metadata -> { 
        
                           int oldPartition = metadata.partitions; 
        
                           for (int i = oldPartition; i < numPartitions; i++) { 
        
                               topicResources().deletePersistentTopicAsync(topicName.getPartition(i)).exceptionally(ex1 -> { 
        
                                   log.warn("[{}] Failed to clean up managedLedger {}", clientAppId(), topicName, 
        
                                           ex1.getCause()); 
        
                                   return null; 
        
                               }); 
        
                           } 
        
                       }).exceptionally(e -> { 
        
                           log.warn("[{}] Failed to clean up managedLedger", topicName, e); 
        
                           return null; 
        
                       }); 
        
                       return null; 
        
                   }); 
        
                   return future; 
        
               }).thenAccept(__ -> result.complete(null)).exceptionally(ex -> { 
        
                   if (force && ex.getCause() instanceof PulsarAdminException.ConflictException) { 
        
                       result.complete(null); 
        
                       return null; 
        
                   } 
        
                   result.completeExceptionally(ex); 
        
                   return null; 
        
               }); 
        
               return result; 
        
           }

The line 4364 updatePartitionedTopicAsync could not reach if the line 4362 createSubscriptions occur an exception of PulsarAdminException$ConflictException: Subscription already exists for topic.

Caused by: org.apache.pulsar.client.admin.PulsarAdminException$ConflictException: Subscription already exists for topic
	at org.apache.pulsar.client.admin.internal.BaseResource.getApiException(BaseResource.java:219) ~[classes/:?]
	at org.apache.pulsar.client.admin.internal.BaseResource$1.failed(BaseResource.java:129) ~[classes/:?]
	at org.glassfish.jersey.client.JerseyInvocation$1.failed(JerseyInvocation.java:839) ~[jersey-client-2.31.jar:?]
	at org.glassfish.jersey.client.JerseyInvocation$1.completed(JerseyInvocation.java:820) ~[jersey-client-2.31.jar:?]
	at org.glassfish.jersey.client.ClientRuntime.processResponse(ClientRuntime.java:229) ~[jersey-client-2.31.jar:?]
	at org.glassfish.jersey.client.ClientRuntime.access$200(ClientRuntime.java:62) ~[jersey-client-2.31.jar:?]
	at org.glassfish.jersey.client.ClientRuntime$2.lambda$response$0(ClientRuntime.java:173) ~[jersey-client-2.31.jar:?]
	at org.glassfish.jersey.internal.Errors$1.call(Errors.java:248) ~[jersey-common-2.31.jar:?]
	at org.glassfish.jersey.internal.Errors$1.call(Errors.java:244) ~[jersey-common-2.31.jar:?]
	at org.glassfish.jersey.internal.Errors.process(Errors.java:292) ~[jersey-common-2.31.jar:?]
	at org.glassfish.jersey.internal.Errors.process(Errors.java:274) ~[jersey-common-2.31.jar:?]
	at org.glassfish.jersey.internal.Errors.process(Errors.java:244) ~[jersey-common-2.31.jar:?]
	at org.glassfish.jersey.process.internal.RequestScope.runInScope(RequestScope.java:288) ~[jersey-common-2.31.jar:?]
	at org.glassfish.jersey.client.ClientRuntime$2.response(ClientRuntime.java:173) ~[jersey-client-2.31.jar:?]
	at org.apache.pulsar.client.admin.internal.http.AsyncHttpConnector.lambda$0(AsyncHttpConnector.java:212) ~[classes/:?]

Modifications

update topic partition metadata when handling ConflictException.

Verifying this change

Make sure that the change passes the CI checks.

This change is a trivial rework / code cleanup without any test coverage.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

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

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

Need to update docs?

doc-not-needed

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java

…cription already exists for topic

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java

HQebupt · 2022-08-26T06:09:48Z

/pulsarbot run-failure-checks

HQebupt · 2022-08-26T07:22:25Z

/pulsarbot run-failure-checks

Jason918

This fix LGTM.
We can also consider add a "force" parameter in admin.topics().createSubscription() to ignore conflict exception when the subscription already exists.

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java

AnonHxy · 2022-08-26T08:08:25Z

LGTM now, left minor commnet :) @HQebupt

HQebupt · 2022-08-26T09:12:32Z

This fix LGTM.
We can also consider add a "force" parameter in admin.topics().createSubscription() to ignore conflict exception when the subscription already exists.

I will continue to pay attention to this point.

HQebupt · 2022-08-26T11:30:59Z

/pulsarbot run-failure-checks

HQebupt · 2022-08-27T05:30:22Z

/pulsarbot run-failure-checks

HQebupt · 2022-08-27T06:18:56Z

/pulsarbot run-failure-checks

…ception: subscription already exists for topic (apache#17251) ### Motivation apache#10374 wants to handle the existing subscriptions when updating partition with `force` command parameter. However, it could not update the partition metadata if the `Subscription already exists for topic` exception comes. Because it miss updating when handling such exception.

Jason918 · 2022-09-04T13:32:01Z

Can you help open a new PR to branch-2.10? There are a lot conflict when cherry-pick directly. @HQebupt

HQebupt · 2022-09-05T13:54:32Z

Can you help open a new PR to branch-2.10? There are a lot conflict when cherry-pick directly. @HQebupt

Sure

Jason918 · 2022-09-07T04:31:05Z

Move release/2.10.2 to #17488

…ception: subscription already exists for topic (#17251) ### Motivation #10374 wants to handle the existing subscriptions when updating partition with `force` command parameter. However, it could not update the partition metadata if the `Subscription already exists for topic` exception comes. Because it miss updating when handling such exception.

HQebupt changed the title ~~fail to update partition meta of topic due to ConflictException: subs~~ [fix][broker]fail to update partition meta of topic due to ConflictException: subscription already exists for topic Aug 24, 2022

Jason918 requested review from Jason918, Technoboy- and mattisonchao August 24, 2022 04:39

Jason918 assigned HQebupt Aug 24, 2022

Jason918 added the type/bug The PR fixed a bug or issue reported a bug label Aug 24, 2022

codelipenghui reviewed Aug 24, 2022

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java Outdated Show resolved Hide resolved

HQebupt requested review from codelipenghui and removed request for Jason918, Technoboy- and mattisonchao August 24, 2022 09:11

fail to update partition meta of topic due to ConflictException: subs…

4a8a48e

…cription already exists for topic

HQebupt force-pushed the fixUpdatePartition branch from a0b7ae8 to 4a8a48e Compare August 24, 2022 09:44

AnonHxy reviewed Aug 25, 2022

View reviewed changes

AnonHxy requested a review from Jason918 August 25, 2022 07:18

AnonHxy assigned HQebupt and unassigned HQebupt Aug 25, 2022

AnonHxy added the release/2.10.2 label Aug 25, 2022

HQebupt added 6 commits August 26, 2022 10:25

address comments

be303f1

address comments

4219d50

address comments

a05524d

address comments

0000386

address comments

ffeece5

address comments

e1f311e

Jason918 approved these changes Aug 26, 2022

View reviewed changes

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Aug 26, 2022

AnonHxy approved these changes Aug 26, 2022

View reviewed changes

AnonHxy reviewed Aug 26, 2022

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java Outdated Show resolved Hide resolved

address comments

bdba626

Merge branch 'master' into fixUpdatePartition

f818e79

Jason918 added the release/2.11.1 label Aug 27, 2022

Jason918 requested review from Technoboy- and mattisonchao August 27, 2022 09:18

codelipenghui approved these changes Aug 28, 2022

View reviewed changes

codelipenghui added this to the 2.12.0 milestone Aug 28, 2022

AnonHxy merged commit 3422ab4 into apache:master Aug 28, 2022

TakaHiR07 mentioned this pull request Aug 31, 2022

[fix][broker]fix catching ConflictException when update topic partition #17374

Merged

5 tasks

HQebupt mentioned this pull request Sep 6, 2022

[Branch-2.10][fix][broker]fail to update partition meta of topic due to ConflictException: subscription already exists for topic #17488

Merged

2 tasks

Jason918 removed the release/2.10.2 label Sep 7, 2022

Technoboy- added the cherry-picked/branch-2.11 label Mar 6, 2023

	private CompletableFuture<Void> updatePartitionedTopic(TopicName topicName, int numPartitions, boolean force) {
	CompletableFuture<Void> result = new CompletableFuture<>();
	createSubscriptions(topicName, numPartitions).thenCompose(__ -> {
	CompletableFuture<Void> future = namespaceResources().getPartitionedTopicResources()
	.updatePartitionedTopicAsync(topicName, p -> new PartitionedTopicMetadata(numPartitions));
	future.exceptionally(ex -> {
	// If the update operation fails, clean up the partitions that were created
	getPartitionedTopicMetadataAsync(topicName, false, false).thenAccept(metadata -> {
	int oldPartition = metadata.partitions;
	for (int i = oldPartition; i < numPartitions; i++) {
	topicResources().deletePersistentTopicAsync(topicName.getPartition(i)).exceptionally(ex1 -> {
	log.warn("[{}] Failed to clean up managedLedger {}", clientAppId(), topicName,
	ex1.getCause());
	return null;
	});
	}
	}).exceptionally(e -> {
	log.warn("[{}] Failed to clean up managedLedger", topicName, e);
	return null;
	});
	return null;
	});
	return future;
	}).thenAccept(__ -> result.complete(null)).exceptionally(ex -> {
	if (force && ex.getCause() instanceof PulsarAdminException.ConflictException) {
	result.complete(null);
	return null;
	}
	result.completeExceptionally(ex);
	return null;
	});
	return result;
	}

[fix][broker]fail to update partition meta of topic due to ConflictException: subscription already exists for topic #17251

[fix][broker]fail to update partition meta of topic due to ConflictException: subscription already exists for topic #17251

Uh oh!

Conversation

HQebupt commented Aug 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

HQebupt commented Aug 26, 2022

Uh oh!

HQebupt commented Aug 26, 2022

Uh oh!

Jason918 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AnonHxy commented Aug 26, 2022

Uh oh!

HQebupt commented Aug 26, 2022

Uh oh!

HQebupt commented Aug 26, 2022

Uh oh!

HQebupt commented Aug 27, 2022

Uh oh!

HQebupt commented Aug 27, 2022

Uh oh!

Jason918 commented Sep 4, 2022

Uh oh!

HQebupt commented Sep 5, 2022

Uh oh!

Jason918 commented Sep 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

HQebupt commented Aug 24, 2022 •

edited

Loading