Issue #1117: handle race in concurrent bundle split #1428

zhaijack · 2018-03-23T14:59:03Z

Motivation

in NamespaceService.splitAndOwnBundle().
The concurrent update of Policies of same namespace will cause some override to bundles value that kept in Zookeeper. And this involves at least 2 rounds of split.
e.g.

At beginning, we have 2 bundles, with partitions:[0x00000000, 0x7fffffff, 0xffffffff],
First round of split: We will split the 2 bundles at the same time. In first bundle split, the NamespaceBundles value that returned by NamespaceBundleFactory.splitBundles() would be:
[0x00000000, 0x3fffffff, 0x7fffffff, 0xffffffff];
while at the same time, the NamespaceBundles value for second bundle split would be:
[0x00000000, 0x7fffffff, 0xbfffffff, 0xffffffff]
Then the 2 split operations, whoever success updateNamespaceBundles last, will override the former one.
Here if second split finally success, it will override the former success value [0x00000000, 0x3fffffff, 0x7fffffff, 0xffffffff], and the final bundles partitions would be: [0x00000000, 0x7fffffff, 0xbfffffff, 0xffffffff]; But we thought it should be [0x00000000, 0x3fffffff, 0x7fffffff, 0xbfffffff, 0xffffffff];
Second round split: e.g. try to split to range [0x00000000, 0x3fffffff], it will not find upper boundary 0x3fffffff in actual [0x00000000, 0x7fffffff, 0xbfffffff, 0xffffffff].
And this will cause the error: 
java.lang.IllegalArgumentException: Invalid upper boundary for bundle at com.google.common.base.Preconditions.checkArgument(Preconditions.java:122)

Modifications

Add version in NamespaceBundles, when update the zookeeper z-node, verify the version, and do read-modify-write retry when there are simultaneous write to the z-node.
Add test for it.

Result

No user behavior will change. This concurrent bug fixed.

Fixes #1117

merlimat

Change looks good, just minor comments

merlimat · 2018-03-23T16:14:50Z

pulsar-zookeeper-utils/src/main/java/org/apache/pulsar/zookeeper/ZooKeeperDataCache.java

+        });
+
+        return future;
+    }


Could this be simplified into:

return cache.getDataAsync(path, this, this).whenComplete((entry, ex) -> { if (ex != null) { cache.asyncInvalidate(path); } });

merlimat · 2018-03-23T16:16:56Z

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


        final CompletableFuture<Void> unloadFuture = new CompletableFuture<>();
+        final int retryTimes = 7;


Move this constant as a private static final int BUNDLE_SPLIT_RETRY_LIMIT = 7 in class

merlimat · 2018-03-23T16:17:59Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java

@@ -1022,7 +1022,18 @@ public AuthenticationService getAuthenticationService() {
    }

    public List<Topic> getAllTopicsFromNamespaceBundle(String namespace, String bundle) {
-        return multiLayerTopicsMap.get(namespace).get(bundle).values();
+        if (multiLayerTopicsMap.get(namespace) == null) {
+            return Lists.newArrayList();


Would Collections.emptyList() work ?

merlimat · 2018-03-23T16:20:52Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java

+            return Lists.newArrayList();
+        }
+
+        if (multiLayerTopicsMap.get(namespace).get(bundle) == null) {


The multiLayerTopicsMap might have been already changed at this point.

The safest approach here would to do something like:

Map<X,Y> map1 = multiLayerTopicsMap.get(namespace); if (map1 == null) { return Collections.emptyList(); } Map<Z,W> map2 = map1.get(bundle); if (map2 == null) { return Collections.emptyList(); } return map2.values();

merlimat · 2018-03-23T16:22:38Z

@rdhabalia Please take a look as well

rdhabalia · 2018-03-23T17:16:58Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/cache/LocalZooKeeperCacheService.java

@@ -105,6 +108,38 @@ public LocalPolicies deserialize(String path, byte[] content) throws Exception {

                return future;
            }
+
+            @Override
+            public CompletableFuture<Optional<Entry<LocalPolicies, Stat>>> getWithStatAsync(String path) {


should we remove getAsync(String path) as it has duplicate logic and it is being used by bundle split only?

Thanks, Seems a lot of other place call it indirectly, remove it will cause error of BrokerServiceTest/testCreateNamespacePolicy at least.
Added it back in latest commit, which removed the duplicated logic.

rdhabalia · 2018-03-23T17:19:56Z

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

+        // If success updateNamespaceBundles, then do invalidateBundleCache and unload.
+        // Else retry splitAndOwnBundleOnceAndRetry.
+        updateFuture.whenCompleteAsync((r, t)-> {
+            if (t != null) {


shouldn't we retry only when we get BadVersion error?

rdhabalia · 2018-03-23T17:25:17Z

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

+                        () -> splitAndOwnBundleOnceAndRetry(bundle, unload, counter, unloadFuture));
+                } else {
+                    // Retry enough, fail this.
+                    String msg2 = format("Finish retry, %s not success update nsBundles, reason %S",


reason %s?

merlimat

👍

zhaijack · 2018-03-24T02:19:37Z

Thanks @merlimat @rdhabalia for your comments, changed it and rebased to catch up master.

zhaijack · 2018-03-26T00:30:06Z

retest this please

merlimat · 2018-03-27T01:09:21Z

@zhaijack Please merge with master that has fix for a very flaky test.

Also it appears there's a test that might be genuiniley failing:

https://builds.apache.org/job/pulsar-pull-request/2290/org.apache.pulsar$pulsar-broker/testReport/org.apache.pulsar.broker.service/BrokerServiceTest/testCreateNamespacePolicy/

I don't remember having seen that test failing before.

zhaijack · 2018-03-27T01:38:37Z

@merlimat thanks, will handle it now

zhaijack · 2018-03-27T04:48:26Z

@merlimat , test fail because of wrongly removed getData in LocalZooKeeperCacheService in commit 2f1e6e1, added it back.

zhaijack force-pushed the issue_1117 branch from d5d9ed5 to ffd2256 Compare March 23, 2018 15:09

merlimat reviewed Mar 23, 2018

View reviewed changes

merlimat requested a review from rdhabalia March 23, 2018 16:22

merlimat assigned zhaijack Mar 23, 2018

merlimat added the type/bug The PR fixed a bug or issue reported a bug label Mar 23, 2018

merlimat added this to the 2.0.0-incubating milestone Mar 23, 2018

rdhabalia reviewed Mar 23, 2018

View reviewed changes

zhaijack force-pushed the issue_1117 branch from ffd2256 to 3b9cc1e Compare March 24, 2018 02:17

merlimat approved these changes Mar 24, 2018

View reviewed changes

zhaijack force-pushed the issue_1117 branch from 3b9cc1e to 66b6320 Compare March 26, 2018 01:47

jiazhai added 3 commits March 27, 2018 10:06

handle race in bundle split

511c934

change following comments

2f1e6e1

rebase master, fix test error

c303722

zhaijack force-pushed the issue_1117 branch from 66b6320 to c303722 Compare March 27, 2018 04:40

merlimat merged commit ea39148 into apache:master Mar 27, 2018

jai1 mentioned this pull request May 2, 2018

Issue #1117: handle race in concurrent bundle split #1710

Merged

massakam mentioned this pull request Aug 9, 2018

Fix NPE when splitting and unloading bundle #2348

Merged

lhotari mentioned this pull request May 5, 2022

splitting bundle error #5510

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #1117: handle race in concurrent bundle split #1428

Issue #1117: handle race in concurrent bundle split #1428

zhaijack commented Mar 23, 2018 •

edited by merlimat

Loading

merlimat left a comment

merlimat Mar 23, 2018

merlimat Mar 23, 2018

merlimat Mar 23, 2018

merlimat Mar 23, 2018

merlimat commented Mar 23, 2018

rdhabalia Mar 23, 2018 •

edited

Loading

zhaijack Mar 27, 2018

rdhabalia Mar 23, 2018

rdhabalia Mar 23, 2018

merlimat left a comment

zhaijack commented Mar 24, 2018

zhaijack commented Mar 26, 2018

merlimat commented Mar 27, 2018

zhaijack commented Mar 27, 2018

zhaijack commented Mar 27, 2018 •

edited

Loading


		final CompletableFuture<Void> unloadFuture = new CompletableFuture<>();
		final int retryTimes = 7;

Issue #1117: handle race in concurrent bundle split #1428

Issue #1117: handle race in concurrent bundle split #1428

Conversation

zhaijack commented Mar 23, 2018 • edited by merlimat Loading

Motivation

Modifications

Result

merlimat left a comment

Choose a reason for hiding this comment

merlimat Mar 23, 2018

Choose a reason for hiding this comment

merlimat Mar 23, 2018

Choose a reason for hiding this comment

merlimat Mar 23, 2018

Choose a reason for hiding this comment

merlimat Mar 23, 2018

Choose a reason for hiding this comment

merlimat commented Mar 23, 2018

rdhabalia Mar 23, 2018 • edited Loading

Choose a reason for hiding this comment

zhaijack Mar 27, 2018

Choose a reason for hiding this comment

rdhabalia Mar 23, 2018

Choose a reason for hiding this comment

rdhabalia Mar 23, 2018

Choose a reason for hiding this comment

merlimat left a comment

Choose a reason for hiding this comment

zhaijack commented Mar 24, 2018

zhaijack commented Mar 26, 2018

merlimat commented Mar 27, 2018

zhaijack commented Mar 27, 2018

zhaijack commented Mar 27, 2018 • edited Loading

zhaijack commented Mar 23, 2018 •

edited by merlimat

Loading

rdhabalia Mar 23, 2018 •

edited

Loading

zhaijack commented Mar 27, 2018 •

edited

Loading