Add load shedding strategy #345

bobbeyreese · 2017-04-11T21:15:16Z

Motivation

ModularLoadManagerImpl was added without load shedding supported. This PR adds a load shedding strategy that will attempt to unload the most expensive bundle from overloaded brokers.

Modifications

The strategy OverloadShedder has been added.
Recently unloaded bundles will not be considered for unloading.
No load shedding will be performed if there is only 1 broker.
No load shedding will be performed on any broker for which there is at most 1 bundle.

Result

Load shedding will now be performed if the conditions checked in LoadManagerShared.isUnloadDisabledInLoadShedding, which was moved from SimpleLoadManagerImpl.

merlimat · 2017-04-12T16:25:19Z

pulsar-broker/src/main/java/com/yahoo/pulsar/broker/loadbalance/LoadData.java

-        this.brokerData = new ConcurrentHashMap<>();
-        this.bundleData = new ConcurrentHashMap<>();
-    }
+	/**


convert tabs --> spaces

merlimat · 2017-04-12T16:47:30Z

...ar-broker/src/main/java/com/yahoo/pulsar/broker/loadbalance/impl/ModularLoadManagerImpl.java

-                                LoadManagerShared.getNamespaceNameFromBundleName(bundle),
-                                LoadManagerShared.getBundleRangeFromBundleName(bundle));
+        if (!LoadManagerShared.isUnloadDisabledInLoadShedding(pulsar)) {
+            if (getAvailableBrokers().size() > 1) {


Instead of indenting everything, consider doing :

if (LoadManagerShared.isUnloadDisabledInLoadShedding(pulsar)) { return; } if (getAvailableBrokers().size() <= 1) { return; } // continue

merlimat · 2017-04-20T20:33:25Z

...ar-broker/src/main/java/com/yahoo/pulsar/broker/loadbalance/impl/ModularLoadManagerImpl.java

+                // map.
+                final Iterator<Map.Entry<String, Long>> recentlyUnloadedIterator = loadData.getRecentlyUnloadedBundles()
+                        .entrySet().iterator();
+                while (recentlyUnloadedIterator.hasNext()) {


This part could be simplified by using a filter like:

loadData.getRecentlyUnloadedBundles().removeIf( e -> e.getValue() < TIMEOUT );

( don't look at the actual check above..)

merlimat · 2017-04-20T20:33:59Z

pulsar-broker/src/main/java/com/yahoo/pulsar/broker/loadbalance/impl/OverloadShedder.java

@@ -0,0 +1,91 @@
+package com.yahoo.pulsar.broker.loadbalance.impl;


copyright header missing

…edding

rdhabalia

LGTM.. just few minor comments

rdhabalia · 2017-04-27T22:25:16Z

...ar-broker/src/main/java/com/yahoo/pulsar/broker/loadbalance/impl/ModularLoadManagerImpl.java

+            return;
+        }
+        if (getAvailableBrokers().size() <= 1) {
+            log.warn("Only 1 broker available: no load shedding will be performed");


may be log.info?

rdhabalia · 2017-04-27T22:44:04Z

pulsar-broker/src/main/java/com/yahoo/pulsar/broker/loadbalance/impl/OverloadShedder.java

+            final LocalBrokerData localData = brokerData.getLocalData();
+            final double maxUsage = localData.getMaxResourceUsage();
+            if (maxUsage >= overloadThreshold) {
+                log.info("Attempting to shed load on {}, which has max resource usage {}%", broker, maxUsage);


Not sure, but should we check if atleast one broker is available (usage is < maxUsage) which can load the bundle. Else, in case if all the brokers reached to maxUsage, they will keep juggling bundles rather they should not unload until at least one broker is available.??

Well, ideally if they were all overloaded, the bundle would have an opportunity to be assigned to a less loaded broker by being unloaded. Then, the grace period would be set highly enough so that juggling would not be a major issue. What are your thoughts?

Then, the grace period would be set highly enough so that juggling would not be a major issue.

but don't we set grace-period at server-startup? will broker change this grace-period dynamically?

I mean that it should be set highly enough on startup -- i.e., maybe something like 5x the load-shedding interval? Of course we could make it dynamic, but I feel like just always having it high makes sense since we would likely very infrequently want to unload the same bundle twice except over a long period of time

rdhabalia · 2017-04-27T22:49:24Z

...ar-broker/src/main/java/com/yahoo/pulsar/broker/loadbalance/impl/ModularLoadManagerImpl.java

+                        final String broker = entry.getKey();
+                        final String bundle = entry.getValue();
+                        log.info("Unloading bundle: {}", bundle);
+                        adminCache.get("http://" + broker).namespaces().unloadNamespaceBundle(


is there any specific reason of having LoadingCache<String, PulsarAdmin> adminCache and not having only one PulsarAdmin? Because, adminClient uses vip and request can go to any broker, but if it is not right broker then it just redirects to correct one. So, I think redirection is less expensive than keeping AdminClient for each broker?

That reasoning seems sound, I kept the admin cache because that's how it was done on SimpleLoadManagerImpl, but since they would not seem to be repercussions for just using one, I will take it out.

rdhabalia · 2017-04-27T22:53:26Z

...ar-broker/src/main/java/com/yahoo/pulsar/broker/loadbalance/impl/ModularLoadManagerImpl.java

@@ -448,19 +445,34 @@ public void disableBroker() throws PulsarServerException {
     */
    @Override
    public synchronized void doLoadShedding() {
+        if (LoadManagerShared.isUnloadDisabledInLoadShedding(pulsar)) {


is it possible to add unit-test case for loadShedding of ModularLoadManagerImpl?

Yes, I will add one

…edding

merlimat

👍

merlimat · 2017-05-02T23:07:04Z

@bobbeyreese Can you resolve the minor conflict with master?

Add load shedding strategy

be945a3

merlimat added the type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages label Apr 11, 2017

merlimat added this to the 1.18 milestone Apr 11, 2017

Resolve conflicts

e4116b7

merlimat reviewed Apr 20, 2017

View reviewed changes

bobbeyreese added 3 commits April 21, 2017 13:32

Merge branch 'master' of https://github.com/yahoo/pulsar into load-sh…

ccd0309

…edding

Merge branch 'master' of https://github.com/yahoo/pulsar into load-sh…

862e0ac

…edding

Fix formatting, add copyright

330014e

rdhabalia reviewed Apr 27, 2017

View reviewed changes

rdhabalia assigned bobbeyreese Apr 27, 2017

bobbeyreese added 2 commits April 28, 2017 13:40

Merge branch 'master' of https://github.com/yahoo/pulsar into load-sh…

786f3b3

…edding

Add unit test nad remove admin cache

e42123c

merlimat approved these changes May 1, 2017

View reviewed changes

Resolve conflicts

bf9d4de

merlimat merged commit 914e428 into apache:master May 2, 2017

hangc0276 pushed a commit to hangc0276/pulsar that referenced this pull request May 26, 2021

format log print (apache#345)

2917ee1

xiaotongwang1 mentioned this pull request Aug 4, 2021

Pulsar 2.7.0+ KOP 2.7.2.x getPartitionedTopicMetadata timeout #11532

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add load shedding strategy #345

Add load shedding strategy #345

bobbeyreese commented Apr 11, 2017

merlimat Apr 12, 2017

merlimat Apr 12, 2017

merlimat Apr 20, 2017

merlimat Apr 20, 2017

rdhabalia left a comment

rdhabalia Apr 27, 2017

rdhabalia Apr 27, 2017

bobbeyreese Apr 27, 2017

rdhabalia Apr 29, 2017

bobbeyreese May 1, 2017

rdhabalia Apr 27, 2017

bobbeyreese Apr 27, 2017

rdhabalia Apr 27, 2017

bobbeyreese Apr 27, 2017

merlimat left a comment

merlimat commented May 2, 2017

		@@ -0,0 +1,91 @@
		package com.yahoo.pulsar.broker.loadbalance.impl;

Add load shedding strategy #345

Add load shedding strategy #345

Conversation

bobbeyreese commented Apr 11, 2017

Motivation

Modifications

Result

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

rdhabalia left a 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

merlimat left a comment

Choose a reason for hiding this comment

merlimat commented May 2, 2017