[improve][broker] PIP-192 Added SplitScheduler and DefaultNamespaceBundleSplitStrategyImpl #19622

heesung-sn · 2023-02-24T03:08:21Z

Master Issue: #16691

Motivation

We will start raising PRs to implement PIP-192, #16691

Modifications

This PR implemented

SplitScheduler
DefaultNamespaceBundleSplitStrategyImpl
SplitManager
and their unit test.

Verifying this change

Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

*Added unit tests.

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

We will have separate PRs to update the Doc later.

Matching PR in forked repository

PR in forked repository: heesung-sn#30

Demogorgon314 · 2023-02-24T08:35:08Z

...ava/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateChannelImpl.java

@@ -847,7 +847,6 @@ protected void splitServiceUnitOnceAndRetry(NamespaceService namespaceService,
            return null;
        });
    }
-


Useless change.

Demogorgon314 · 2023-02-25T05:10:39Z

...e/pulsar/broker/loadbalance/extensions/strategy/DefaultNamespaceBundleSplitStrategyImpl.java

+        var bundleHighTrafficIterator =
+                bundleHighTrafficFrequency.entrySet().iterator();
+        while (bundleHighTrafficIterator.hasNext()) {
+            String bundle = bundleHighTrafficIterator.next().getKey();
+            if (!bundleStatsMap.containsKey(bundle)) {
+                bundleHighTrafficIterator.remove();
+            }
+        }


Suggested change

var bundleHighTrafficIterator =

bundleHighTrafficFrequency.entrySet().iterator();

while (bundleHighTrafficIterator.hasNext()) {

String bundle = bundleHighTrafficIterator.next().getKey();

if (!bundleStatsMap.containsKey(bundle)) {

bundleHighTrafficIterator.remove();

}

}

bundleHighTrafficFrequency.keySet().retainAll(bundleStatsMap.keySet());

Demogorgon314 · 2023-02-25T05:27:43Z

.../src/main/java/org/apache/pulsar/broker/loadbalance/extensions/scheduler/SplitScheduler.java

+                for (SplitDecision decision : decisions) {
+                    if (decision.getLabel() == Success) {
+                        var split = decision.getSplit();
+                        futures.add(serviceUnitStateChannel.publishSplitEventAsync(split)


Do we need to wait until the split (Received Deleted message) is finished,

Great point. I see that UnloadScheduler does not wait for unload completion now.

However, I agree that we better wait for completion (wait on channel.getOwnerAsync) to confirm the completion on the same dedicated thread.

The class name, SplitScheduler would sound counterintuitive if the logic schedules splits and waits for completion. Perhaps, the better name could be SplitManager.

Do we want this class name change and adding the waiting logic in this PR, or a separate PR(also refactoring UnloadScheduler)?

How about we add a method like this?
https://github.com/apache/pulsar/pull/19538/files#diff-b9a27acaccf9d79fd9c1e344f940ade6565f3c1b9fa8000e62d0c116e311a113R134

Let's continue this discussion in your PR, as you have a proposal for this waiting logic in that PR.

heesung-sn · 2023-03-01T05:03:32Z

I will add the waiting logic once this PR gets merged. #19538

...e/pulsar/broker/loadbalance/extensions/strategy/DefaultNamespaceBundleSplitStrategyImpl.java

Demogorgon314 · 2023-03-02T06:03:59Z

.../src/main/java/org/apache/pulsar/broker/loadbalance/extensions/scheduler/SplitScheduler.java

+        }
+
+        if (counter.updatedAt() > counterLastUpdatedAt) {
+            splitMetrics.set(counter.toMetrics(pulsar.getAdvertisedAddress()));


Why don't we use brokerRegistry().getBrokerId()?

To be consistent with other metrics code, here we use pulsar.getAdvertisedAddress()

Demogorgon314 · 2023-03-02T08:16:38Z

.../src/main/java/org/apache/pulsar/broker/loadbalance/extensions/scheduler/SplitScheduler.java

+            }
+        }
+
+        if (counter.updatedAt() > counterLastUpdatedAt) {


Should update when FutureUtil.waitForAll(futures).whenComplete.

Updated to make syncWaiting after FutureUtil.waitForAll .

Demogorgon314 · 2023-03-02T08:18:42Z

.../src/main/java/org/apache/pulsar/broker/loadbalance/extensions/scheduler/SplitScheduler.java

+                }
+                FutureUtil.waitForAll(futures).exceptionally(ex -> {
+                    log.error("Failed to wait for split events to persist.", ex);
+                    counter.update(Failure, Unknown);


Already updated when serviceUnitStateChannel.publishSplitEventAsync(split) failure. Here is an unnecessary update.

Demogorgon314 · 2023-03-08T08:53:05Z

...oker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/manager/SplitManager.java

+                }
+            });
+            return new InFlightSplitRequest(decision, future);
+        }).future);


We should update the counter here, because the eventPubFuture might be failed.

Moved the pub failure handling logic to SplitManager.

Demogorgon314 · 2023-03-09T02:28:38Z

...oker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/manager/SplitManager.java

+                .exceptionally(e -> {
+                    log.error("Failed to publish the bundle split event for bundle:{}. Skipping wait.", bundle);
+                    counter.update(Failure, Unknown);
+                    return null;


Using exceptionally will cause the returned future to lose the exception info. We should use whenComplete here.

Demogorgon314 · 2023-03-09T03:45:48Z

...oker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/manager/SplitManager.java

+            var future = inFlightSplitRequest.future;
+            if (!future.isDone()) {
+                if (ex != null) {
+                    counter.update(Failure, Unknown);


If we update the counter here, when complete exception it will updates twice

I suggest moving the counter update to waitAsync.

Can you clarify how we are updating this failure counter twice?

oh I see. I think we need to clean this part.

Nice catch. Updated.

Demogorgon314 · 2023-03-09T05:13:49Z

...oker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/manager/SplitManager.java

+                            log.warn("Timed out while waiting for the bundle split event: {}", bundle, ex);
+                        }
+                    });
+                    return new InFlightSplitRequest(decision, future);


Now we don't need InFlightSplitRequest obj right? Since we already pass the decision in whenComplete

Agreed. Updated.

gaoran10

LGTM

Technoboy- · 2023-03-13T06:20:22Z

/pulsarbot run-failure-checks

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Feb 24, 2023

heesung-sn mentioned this pull request Feb 24, 2023

PIP-192: New Pulsar Broker Load Balancer #16691

Closed

Demogorgon314 reviewed Feb 25, 2023

View reviewed changes

gaoran10 reviewed Mar 1, 2023

View reviewed changes

...e/pulsar/broker/loadbalance/extensions/strategy/DefaultNamespaceBundleSplitStrategyImpl.java Show resolved Hide resolved

Demogorgon314 reviewed Mar 2, 2023

View reviewed changes

heesung-sn added 2 commits March 7, 2023 15:39

[improve][broker] PIP-192 Added Split Scheduler

42c23e6

Added SplitManager

c856344

heesung-sn force-pushed the pip-192-split-shceduler branch from 73be2ba to c856344 Compare March 8, 2023 02:14

Demogorgon314 reviewed Mar 8, 2023

View reviewed changes

Updated pub failure counter

02c9274

Demogorgon314 reviewed Mar 9, 2023

View reviewed changes

resolved comment

0d88959

Demogorgon314 reviewed Mar 9, 2023

View reviewed changes

fixed failure count

1ac2868

heesung-sn force-pushed the pip-192-split-shceduler branch from e71ea9f to 1ac2868 Compare March 9, 2023 05:22

removed InflightReuqest record

0d245a8

Demogorgon314 approved these changes Mar 9, 2023

View reviewed changes

Demogorgon314 assigned heesung-sn Mar 9, 2023

Demogorgon314 added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/broker ready-to-test labels Mar 9, 2023

Demogorgon314 closed this Mar 9, 2023

Demogorgon314 reopened this Mar 9, 2023

Demogorgon314 requested review from gaoran10 and Technoboy- March 9, 2023 05:50

gaoran10 approved these changes Mar 9, 2023

View reviewed changes

Demogorgon314 mentioned this pull request Mar 10, 2023

[improve][broker] PIP-192: Make split admin API functional #19773

Merged

4 tasks

Demogorgon314 merged commit 9a85dea into apache:master Mar 14, 2023

heesung-sn deleted the pip-192-split-shceduler branch April 2, 2024 17:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[improve][broker] PIP-192 Added SplitScheduler and DefaultNamespaceBundleSplitStrategyImpl #19622

[improve][broker] PIP-192 Added SplitScheduler and DefaultNamespaceBundleSplitStrategyImpl #19622

heesung-sn commented Feb 24, 2023 •

edited

Loading

Demogorgon314 Feb 24, 2023

heesung-sn Mar 8, 2023

Demogorgon314 Feb 25, 2023

heesung-sn Mar 8, 2023

Demogorgon314 Feb 25, 2023

heesung-sn Feb 25, 2023

Demogorgon314 Feb 25, 2023

heesung-sn Feb 25, 2023

heesung-sn commented Mar 1, 2023

Demogorgon314 Mar 2, 2023

heesung-sn Mar 8, 2023

Demogorgon314 Mar 9, 2023

Demogorgon314 Mar 2, 2023

heesung-sn Mar 8, 2023

Demogorgon314 Mar 2, 2023

heesung-sn Mar 8, 2023

Demogorgon314 Mar 8, 2023

heesung-sn Mar 8, 2023

Demogorgon314 Mar 9, 2023

heesung-sn Mar 9, 2023

Demogorgon314 Mar 9, 2023

Demogorgon314 Mar 9, 2023

heesung-sn Mar 9, 2023

heesung-sn Mar 9, 2023

heesung-sn Mar 9, 2023

Demogorgon314 Mar 9, 2023

heesung-sn Mar 9, 2023

gaoran10 left a comment

Technoboy- commented Mar 13, 2023

[improve][broker] PIP-192 Added SplitScheduler and DefaultNamespaceBundleSplitStrategyImpl #19622

[improve][broker] PIP-192 Added SplitScheduler and DefaultNamespaceBundleSplitStrategyImpl #19622

Conversation

heesung-sn commented Feb 24, 2023 • edited Loading

Motivation

Modifications

Verifying this change

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

Documentation

Matching PR in forked repository

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heesung-sn commented Mar 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaoran10 left a comment

Choose a reason for hiding this comment

Technoboy- commented Mar 13, 2023

heesung-sn commented Feb 24, 2023 •

edited

Loading