[improve][broker]Create missed subscriptions for createMissedPartitions #17946

Pomelongan · 2022-10-05T15:14:55Z

Motivation

org.apache.pulsar.broker.admin.impl.PersistentTopicsBase#internalCreateMissedPartitions lacks the logic to create subscriptions.

Modifications

add org.apache.pulsar.broker.admin.impl.PersistentTopicsBase#createMissedSubscriptionsAsync
add the logic for creating missed subscriptions for org.apache.pulsar.broker.admin.impl.PersistentTopicsBase#internalCreateMissedPartitions

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 the box was checked, please highlight the changes

Documentation

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

Matching PR in forked repository

PR in forked repository: Pomelongan#9

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

Jason918

LGTM, Can we add a unit test to verify this?

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

Pomelongan · 2022-10-08T09:31:15Z

LGTM, Can we add a unit test to verify this?

OK, have update UT, PTAL @Jason918

Jason918

LGTM

codelipenghui · 2022-10-10T03:14:18Z

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

+            result.completeExceptionally(e1);
+            return result;
+        }
+        admin.topics().getStatsAsync(topicName.getPartition(0).toString()).thenAccept(stats -> {


Nice catch!

The partition-0 maybe also created by the CreateMissedPartitions, I think we should list the subscriptions of the partitioned topic.

And for Pulsar, users can subscription to a specific partition, which means partition-0, partition-1 might have different subscriptions.

The partition-0 maybe also created by the CreateMissedPartitions, I think we should list the subscriptions of the partitioned topic.

And for Pulsar, users can subscription to a specific partition, which means partition-0, partition-1 might have different subscriptions.

I think we can add a parameter to specify the subscription name list to be created, which can solve these two problems at the same time. How about this？

@Pomelongan For the second one, it's not a problem. The partitions can have different subscriptions. The challenge is that we don't have subscriptions for the partitioned topic, right?

Hmmm, IMO, we can update the documentation to remind the user that "If you found the partitions is missed, you should use create missed partitions command to fix the missed partition issue and try to subscribe to the partitioned topic again to repair the part of the subscription missed issue"

The challenge is that we don't have subscriptions for the partitioned topic, right?

@codelipenghui Yes, adding a parameter to specify the subscription list to be created can solve this problem. I have updated the code,PTAL

Hmmm, IMO, we can update the documentation to remind the user that "If you found the partitions is missed, you should use create missed partitions command to fix the missed partition issue and try to subscribe to the partitioned topic again to repair the part of the subscription missed issue"

Okay, I have updated the document.

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

pulsar-client-tools-test/src/test/java/org/apache/pulsar/admin/cli/PulsarAdminToolTest.java

pulsar-broker/src/test/java/org/apache/pulsar/client/api/PartitionCreationTest.java

Pomelongan · 2022-10-19T06:26:17Z

I have updated based on comments.PTAL @AnonHxy

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

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

codelipenghui · 2022-11-01T04:43:26Z

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

+            for (int i = 0; i < numPartitions; i++) {
+                CompletableFuture<Void> future = new CompletableFuture<>();
+                admin.topics().createSubscriptionAsync(topicName.getPartition(i).toString(), subscription,
+                        MessageId.latest).whenComplete((__, ex) -> {
+                    if (ex == null) {
+                        future.complete(null);
+                    } else {
+                        if (ex instanceof PulsarAdminException.ConflictException) {
+                            future.complete(null);
+                        } else {
+                            future.completeExceptionally(ex);
+                        }
+                    }
+                });
+                subscriptionFutures.add(future);
+            }


We have internalCreateSubscription method which will handle the subscription creation. Can we just use the existing method to avoid duplicated code?

@codelipenghui Okay～I have updated, PTAL.
Calling internalCreateSubscription directly requires implementing the AsyncResponse interface first, which is troublesome. So I try to extract the common part of the two methods to avoid duplicated code.

…ase#internalCreateMissedPartitions

…csBase#internalCreateMissedPartitions

github-actions · 2022-12-13T02:05:06Z

The pr had no activity for 30 days, mark with Stale label.

Jason918 reviewed Oct 6, 2022

View reviewed changes

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

Jason918 reviewed Oct 7, 2022

View reviewed changes

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

Pomelongan force-pushed the update_createMissedPartitions branch from f9c629a to a3a4801 Compare October 8, 2022 09:15

Jason918 approved these changes Oct 10, 2022

View reviewed changes

Jason918 requested review from mattisonchao and Technoboy- October 10, 2022 02:15

Jason918 assigned Pomelongan Oct 10, 2022

Jason918 requested a review from codelipenghui October 10, 2022 02:16

Jason918 added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/broker labels Oct 10, 2022

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

Jason918 added this to the 2.12.0 milestone Oct 10, 2022

codelipenghui requested changes Oct 10, 2022

View reviewed changes

Pomelongan force-pushed the update_createMissedPartitions branch 2 times, most recently from f03b282 to 42fe79e Compare October 18, 2022 11:48

github-actions bot added doc-complete Your PR changes impact docs and the related docs have been already added. and removed doc-not-needed Your PR changes do not impact docs labels Oct 18, 2022

AnonHxy reviewed Oct 18, 2022

View reviewed changes

Pomelongan force-pushed the update_createMissedPartitions branch from 42fe79e to 10a7fa9 Compare October 19, 2022 03:00

AnonHxy changed the title ~~[improve][broker]add the logic for creating missed subscriptions for createMissedPartitions~~ [improve][broker]Create missed subscriptions for createMissedPartitions Oct 19, 2022

AnonHxy reviewed Oct 19, 2022

View reviewed changes

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

Pomelongan force-pushed the update_createMissedPartitions branch 2 times, most recently from 9f8e471 to 0a112d1 Compare October 21, 2022 08:49

AnonHxy reviewed Oct 22, 2022

View reviewed changes

pulsar-broker-common/src/main/java/org/apache/pulsar/broker/resources/NamespaceResources.java Outdated Show resolved Hide resolved

Pomelongan force-pushed the update_createMissedPartitions branch from 0a112d1 to 5fdb237 Compare October 31, 2022 06:58

AnonHxy approved these changes Nov 1, 2022

View reviewed changes

codelipenghui reviewed Nov 1, 2022

View reviewed changes

Pomelongan force-pushed the update_createMissedPartitions branch from 5fdb237 to e12b53e Compare November 3, 2022 15:49

Pomelongan force-pushed the update_createMissedPartitions branch from e12b53e to a10b126 Compare November 4, 2022 05:59

huangzegui added 6 commits November 12, 2022 16:59

add the logic for creating missed subscriptions for PersistentTopicsB…

2fbb4a8

…ase#internalCreateMissedPartitions

update UT

b0f82b2

update comments

871429a

update the logic for creating missed subscriptions for PersistentTopi…

7fdf754

…csBase#internalCreateMissedPartitions

update comments

0ee2f24

avoid duplicated code

bb53b1e

Pomelongan force-pushed the update_createMissedPartitions branch from a10b126 to bb53b1e Compare November 12, 2022 09:01

github-actions bot added the Stale label Dec 13, 2022

poorbarcode modified the milestones: 3.0.0, 3.1.0 Apr 10, 2023

Technoboy- modified the milestones: 3.1.0, 3.2.0 Jul 31, 2023

Technoboy- modified the milestones: 3.2.0, 3.3.0 Dec 22, 2023

coderzc modified the milestones: 3.3.0, 3.4.0 May 8, 2024

lhotari modified the milestones: 4.0.0, 4.1.0 Oct 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[improve][broker]Create missed subscriptions for createMissedPartitions #17946

[improve][broker]Create missed subscriptions for createMissedPartitions #17946

Pomelongan commented Oct 5, 2022 •

edited

Loading

Jason918 left a comment

Pomelongan commented Oct 8, 2022

Jason918 left a comment

codelipenghui Oct 10, 2022 •

edited

Loading

Pomelongan Oct 14, 2022

codelipenghui Oct 17, 2022

Pomelongan Oct 18, 2022

Pomelongan commented Oct 19, 2022

codelipenghui Nov 1, 2022

Pomelongan Nov 3, 2022

github-actions bot commented Dec 13, 2022

[improve][broker]Create missed subscriptions for createMissedPartitions #17946

Are you sure you want to change the base?

[improve][broker]Create missed subscriptions for createMissedPartitions #17946

Conversation

Pomelongan commented Oct 5, 2022 • edited Loading

Motivation

Modifications

Verifying this change

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

Documentation

Matching PR in forked repository

Jason918 left a comment

Choose a reason for hiding this comment

Pomelongan commented Oct 8, 2022

Jason918 left a comment

Choose a reason for hiding this comment

codelipenghui Oct 10, 2022 • edited Loading

Choose a reason for hiding this comment

Pomelongan Oct 14, 2022

Choose a reason for hiding this comment

codelipenghui Oct 17, 2022

Choose a reason for hiding this comment

Pomelongan Oct 18, 2022

Choose a reason for hiding this comment

Pomelongan commented Oct 19, 2022

codelipenghui Nov 1, 2022

Choose a reason for hiding this comment

Pomelongan Nov 3, 2022

Choose a reason for hiding this comment

github-actions bot commented Dec 13, 2022

Pomelongan commented Oct 5, 2022 •

edited

Loading

codelipenghui Oct 10, 2022 •

edited

Loading