[fix][broker] Update new bundle-range to policies after bundle split #17797

lordcheng10 · 2022-09-22T08:44:20Z

Motivation

After the bundle is split, update the new bundle information to admin/policies.

Documentation

doc-required
(Your PR needs to update docs and you will update later)
doc-not-needed
(Please explain why)
doc
(Your PR contains doc changes)
doc-complete
(Docs have been already added)

Matching PR in forked repository

PR in forked repository: lordcheng10#17

mattisonchao · 2022-09-23T01:58:11Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java

                                    for (NamespaceBundle sBundle : splittedBundles.getRight()) {
                                        checkNotNull(ownershipCache.tryAcquiringOwnership(sBundle));
                                    }
+                                    updateNamespaceBundlesForPolicies(nsname, splittedBundles.getLeft())


What about update updateNamespaceBundlesForPolicies success but updateNamespaceBundles fail?

Modify the execution order of the two methods to ensure that updateNamespaceBundlesForPolicies is executed after updateNamespaceBundles is completed.
@mattisonchao PTAL,thanks!

pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java

Jason918 · 2022-09-30T08:28:15Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java

+                                                                + "NamespaceBundle: %s due to %s",
+                                                        nsname.toString(), bundle.getBundleRange(), e.getMessage());
+                                                LOG.warn(msg);
+                                                updateFuture.completeExceptionally(


I wonder what will happen if updateFuture completeExceptionally here and complete success in L896(after updateNamespaceBundles success). There is no executing order guarantee for these two ops.

I wonder what will happen if updateFuture completeExceptionally here and complete success in L896(after updateNamespaceBundles success).

Local policies were updated successfully, but policies update failed.

There is no executing order guarantee for these two ops.

I can add an order guarantee: updateNamespaceBundlesForPolicies is only executed after updateNamespaceBundles is executed successfully.

I add an order guarantee: updateNamespaceBundlesForPolicies is only executed after updateNamespaceBundles is executed successfully!

@Jason918 PTAL,thanks!

Jason918 · 2022-10-01T09:11:54Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java

@@ -884,15 +884,28 @@ void splitAndOwnBundleOnceAndRetry(NamespaceBundle bundle,
                                            .thenRun(() -> {
                                                bundleFactory.invalidateBundleCache(bundle.getNamespaceObject());
                                                updateFuture.complete(splittedBundles.getRight());


It seems updateFuture should complete after updateNamespaceBundlesForPolicies completes.

Fixed,PTAL,thanks! @Jason918

Jason918

LGTM

eolivelli

Can we add some test to cover this change and also to demonstrate the problem?

lordcheng10 · 2022-10-04T01:26:46Z

Can we add some test to cover this change and also to demonstrate the problem?

OK

lordcheng10 · 2022-10-05T09:00:03Z

Can we add some test to cover this change and also to demonstrate the problem?

Fixed! PTAL,thanks! @eolivelli
update NamespaceServiceTest#testSplitAndOwnBundles to check policies updating.

eolivelli

Lgtm

lordcheng10 · 2022-10-06T03:12:28Z

CI passed: lordcheng10#17

mattisonchao

LGTM

lordcheng10 · 2022-10-08T02:08:37Z

@codelipenghui @Technoboy- PTAL,thanks!

lordcheng10 · 2022-10-08T02:09:30Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-10-08T02:18:32Z

@aloyszhang PTAL,thanks!

lordcheng10 · 2022-10-08T03:41:36Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-10-08T04:36:06Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-10-08T06:03:59Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-10-08T07:18:36Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-10-08T08:25:35Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-10-08T11:55:04Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-10-08T12:57:45Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-10-09T01:05:56Z

@codelipenghui @Technoboy- PTAL,thanks!

lordcheng10 · 2022-10-09T01:07:50Z

@AnonHxy PTAL,thanks!

pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java

AnonHxy · 2022-10-09T10:47:21Z

LGTM now

lordcheng10 · 2022-10-09T12:02:33Z

/pulsarbot run-failure-checks

…pache#17797) Co-authored-by: leolinchen <leolinchen@tencent.com>

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

lordcheng10 mentioned this pull request Sep 22, 2022

[run-tests] [fix][broker] Update new bundle-range to policies after bundle split lordcheng10/pulsar#17

Closed

codelipenghui assigned lordcheng10 Sep 22, 2022

codelipenghui added this to the 2.12.0 milestone Sep 22, 2022

mattisonchao reviewed Sep 23, 2022

View reviewed changes

Jason918 reviewed Sep 23, 2022

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java Outdated Show resolved Hide resolved

lordcheng10 requested review from Jason918 and mattisonchao and removed request for Jason918 September 30, 2022 03:58

Jason918 reviewed Sep 30, 2022

View reviewed changes

Jason918 requested changes Oct 1, 2022

View reviewed changes

lordcheng10 requested review from Jason918 and removed request for mattisonchao October 2, 2022 01:54

Jason918 approved these changes Oct 2, 2022

View reviewed changes

lordcheng10 force-pushed the update_policies_after_split branch from 6562742 to 438750e Compare October 2, 2022 07:56

lordcheng10 requested a review from mattisonchao October 2, 2022 07:57

eolivelli requested changes Oct 2, 2022

View reviewed changes

lordcheng10 requested review from eolivelli and removed request for mattisonchao October 5, 2022 09:03

eolivelli approved these changes Oct 5, 2022

View reviewed changes

mattisonchao approved these changes Oct 7, 2022

View reviewed changes

update namespace bundle info for policies when splitting bundle

27c16c0

lordcheng10 force-pushed the update_policies_after_split branch from 18136c1 to 27c16c0 Compare October 8, 2022 14:03

AnonHxy reviewed Oct 9, 2022

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java Outdated Show resolved Hide resolved

aloyszhang approved these changes Oct 9, 2022

View reviewed changes

AnonHxy reviewed Oct 9, 2022

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java Show resolved Hide resolved

use thenCompose replace thenRun

fe19857

AnonHxy approved these changes Oct 9, 2022

View reviewed changes

AnonHxy merged commit 7df4ee9 into apache:master Oct 9, 2022

Technoboy- mentioned this pull request Oct 12, 2022

[fix][test] Fix flaky test testSplitBundleForMultiTimes. #17993

Merged

1 task

Technoboy- added cherry-picked/branch-2.11 release/2.11.2 labels Jul 5, 2023

shibd pushed a commit to shibd/pulsar that referenced this pull request Jul 16, 2023

[fix][broker] Update new bundle-range to policies after bundle split (a…

43dcccc

…pache#17797) Co-authored-by: leolinchen <leolinchen@tencent.com>

[fix][broker] Update new bundle-range to policies after bundle split #17797

[fix][broker] Update new bundle-range to policies after bundle split #17797

Uh oh!

Conversation

lordcheng10 commented Sep 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Documentation

Matching PR in forked repository

Uh oh!

mattisonchao Sep 23, 2022

Choose a reason for hiding this comment

Uh oh!

lordcheng10 Oct 1, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Jason918 Sep 30, 2022

Choose a reason for hiding this comment

Uh oh!

lordcheng10 Oct 1, 2022

Choose a reason for hiding this comment

Uh oh!

lordcheng10 Oct 1, 2022

Choose a reason for hiding this comment

Uh oh!

Jason918 Oct 1, 2022

Choose a reason for hiding this comment

Uh oh!

lordcheng10 Oct 1, 2022

Choose a reason for hiding this comment

Uh oh!

Jason918 left a comment

Choose a reason for hiding this comment

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

lordcheng10 commented Oct 4, 2022

Uh oh!

lordcheng10 commented Oct 5, 2022

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

lordcheng10 commented Oct 6, 2022

Uh oh!

mattisonchao left a comment

Choose a reason for hiding this comment

Uh oh!

lordcheng10 commented Oct 8, 2022

Uh oh!

lordcheng10 commented Oct 8, 2022

Uh oh!

lordcheng10 commented Oct 8, 2022

Uh oh!

lordcheng10 commented Oct 8, 2022

Uh oh!

lordcheng10 commented Oct 8, 2022

Uh oh!

lordcheng10 commented Oct 8, 2022

Uh oh!

lordcheng10 commented Oct 8, 2022

Uh oh!

lordcheng10 commented Oct 8, 2022

Uh oh!

lordcheng10 commented Oct 8, 2022

Uh oh!

lordcheng10 commented Oct 8, 2022

Uh oh!

lordcheng10 commented Oct 9, 2022

Uh oh!

lordcheng10 commented Oct 9, 2022

Uh oh!

Uh oh!

Uh oh!

AnonHxy commented Oct 9, 2022

Uh oh!

lordcheng10 commented Oct 9, 2022

Uh oh!

lordcheng10 commented Sep 22, 2022 •

edited

Loading