Introduce new load manager API #303

bobbeyreese · 2017-03-21T23:34:57Z

Motivation

The current load balancer implementation has demonstrated poor performance in some scenarios, particularly when doing a cold restart or when the load reaches unexpected heights at some brokers. This PR attempts to first address the cold start issue.

Modifications

Added third load balancing strategy based on least messages per second.
Added load simulation servers, which contain producers and consumers used to simulate load.
- com.yahoo.pulsar.testclient.LoadSimulationServer
- pulsar-perf simulation-server <options>
Added controller to send commands to load simulation servers in a shell, such as adding (potentially groups of) producers/consumers, changing message rates/sizes, and stopping them.
- com.yahoo.pulsar.testclient.LoadSimulationController
- pulsar-perf simulation-controller <options>
- Can also run scripts.
Added broker monitor which will watch and print load report information.
- com.yahoo.pulsar.testclient.BrokerMonitor
- pulsar-perf monitor <options>
- Also available in new API: com.yahoo.pulsar.testclient.NewBrokerMonitor and pulsar-perf new-monitor <options>
Added proposal for new load balancing API which is simpler, more modular, and written with less code.
- Interface: com.yahoo.pulsar.broker.loadbalance.NewLoadManager
- Implementation: com.yahoo.pulsar.broker.loadbalance.impl.NewLoadManagerImpl
- May implement LoadManager via com.yahoo.pulsar.broker.loadbalance.impl.NewLoadManagerWrapper
- Added broker filter pipeline, a sequence of objects that blacklist brokers for selection in exceptional cases (e.g., high CPU).
  - com.yahoo.pulsar.broker.loadbalance.BrokerFilter
  - Has no current implementations.
- Added new placement strategy API, which uses all the data available to the leader broker to make bundle assignment decisions.
  - com.yahoo.pulsar.broker.loadbalance.NewPlacementStrategy
  - Currently implemented only by com.yahoo.pulsar.broker.loadbalance.impl.LeastLongTermMessageRate
- Added load shedding strategy, which uses all the information available to the leader broker to decide if any bundles should be unloaded.
  - com.yahoo.pulsar.broker.loadbalance.LoadSheddingStrategy
  - Implemented only by com.yahoo.pulsar.broker.loadbalance.impl.DeviationShedder (untested, abstract)
Added dynamically configurable fields which allow us to switch between strategies and managers (i.e., between old and new API).
Added allocated and preallocated data to the load report, as well as the bundle gains/losses since the last update.

Result

We will have the option to use, configure, and switch between a new load balancing strategy.
We will be able to more easily simulation load and test load balancing strategies.
We will be able to use the old load balancer if we want to.

yahoocla · 2017-03-21T23:34:59Z

Thank you for submitting this pull request, however I do not see a valid CLA on file for you. Before we can merge this request please visit https://yahoocla.herokuapp.com/ and agree to the terms. Thanks! 😄

merlimat

Just added few comments on non-substantial aspects. Will go through it more in deep later.

merlimat · 2017-03-21T23:37:07Z

conf/broker.conf

@@ -293,3 +293,30 @@ keepAliveIntervalSeconds=30

 # How often broker checks for inactive topics to be deleted (topics with no subscriptions and no one connected)
 brokerServicePurgeInactiveFrequencyInSeconds=60
+
+# Number of samples to use for short term time window
+numShortSamples=50


Group all the settings by using a common prefix. eg: something like brokerLoadManagedXYZ...

merlimat · 2017-03-21T23:38:50Z

conf/broker.conf

+defaultMsgRateOut=50
+
+# Name of load manager to use
+loadManagerName=SimpleLoadManager


Use full class name

merlimat · 2017-03-21T23:40:42Z

conf/broker.conf

+numShortSamples=50
+
+# Number of samples to use for long term time window
+numLongSamples=1000


try not to abbreviate too much the config variable names. For example this could be called brokerLoadManagerNumberOfSamplesLongTermWindow=1000

merlimat · 2017-03-21T23:43:12Z

pulsar-broker-common/src/main/java/com/yahoo/pulsar/broker/ServiceConfiguration.java

    // Percentage of change to trigger load report update
    private int loadBalancerReportUpdateThresholdPercentage = 10;
    // maximum interval to update load report
-    private int loadBalancerReportUpdateMaxIntervalMinutes = 15;
+    private int loadBalancerReportUpdateMaxIntervalMinutes = 1;


By lowering this interval to 1min, every broker is going to update its own load report every 1min, even if there were no substantial changes in the traffic.

Whenever 1 broker updates the z-node, all the other broker will get the watch and reload that load report and redo the ranking. That could be a bit expensive and it was the reason to wait up to 15min to update, in the case the load doesn't change more than X%

This makes sense, I will revert the default back to 15 minutes.

merlimat · 2017-03-21T23:46:40Z

pulsar-broker/src/main/java/com/yahoo/pulsar/broker/PulsarService.java

 import java.util.concurrent.locks.Condition;
 import java.util.concurrent.locks.ReentrantLock;
 import java.util.function.Supplier;

 import org.apache.bookkeeper.mledger.ManagedLedgerFactory;
 import org.apache.bookkeeper.util.OrderedSafeExecutor;
-import org.apache.zookeeper.ZooKeeper;
+import org.apache.bookkeeper.util.ZkUtils;
+import org.apache.zookeeper.*;


Please use Eclipse formatter profile and do not group the includes

merlimat · 2017-03-22T00:01:51Z

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

                } catch (KeeperException.NodeExistsException e) {
                    // ignore the exception, node might be present already
                }
            }
-
+            Thread.sleep(5000);


Is this sleep needed?

merlimat · 2017-03-22T00:02:00Z

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

            LoadReport loadReport = null;
+            Thread.sleep(5000);


merlimat · 2017-03-22T00:04:38Z

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

-                "Not enough of primaries [{}] available for namespace - [{}], "
-                    + "adding shared [{}] as possible candidate owners",
-                primaries.size(), namespace.toString(), shared.size());
+                    "Not enough of primaries [{}] available for namespace - [{}], "


Please use the eclipse formatter to minimize formatting diffs

merlimat · 2017-03-22T00:05:52Z

bin/pulsar-perf

@@ -137,6 +137,14 @@ if [ "$COMMAND" == "produce" ]; then
    exec $JAVA $OPTS com.yahoo.pulsar.testclient.PerformanceProducer --conf-file $PULSAR_PERFTEST_CONF "$@"
 elif [ "$COMMAND" == "consume" ]; then
    exec $JAVA $OPTS com.yahoo.pulsar.testclient.PerformanceConsumer --conf-file $PULSAR_PERFTEST_CONF "$@"
+elif [ "$COMMAND" == "monitor" ]; then
+    exec $JAVA $OPTS com.yahoo.pulsar.testclient.BrokerMonitor "$@"


What is the difference between BrokerMonitor and NewBrokerMonitor ?

The difference is that one monitors data when SimpleLoadManagerImpl is used and the other monitors data when ModularLoadManagerImpl is used. I have renamed them to SimpleLoadManagerBrokerMonitor and ModularLoadManagerBrokerMonitor to better reflect the difference.

merlimat · 2017-03-22T00:07:18Z

bin/pulsar-perf

+elif [ "$COMMAND" == "simulation-server" ]; then
+    exec $JAVA $OPTS com.yahoo.pulsar.testclient.LoadSimulationServer "$@"
+elif [ "$COMMAND" == "simulation-controller" ]; then
+    exec $JAVA $OPTS com.yahoo.pulsar.testclient.LoadSimulationController "$@"


also update the pulsar_help() above to add the new commands in the help message

merlimat

Overall looks very nice. Just few comments and formatting (use Eclipse with the formatting profile from src/formatter.xml).

Once this is merged, then it would also be good to have a document in the "Internal Docs" section to explain the mechanism behind the load manager and, even nicer, to have some visualization of the load distribution comparing old vs new load manager.

merlimat · 2017-03-24T04:27:50Z

pulsar-broker-common/src/main/java/com/yahoo/pulsar/broker/ServiceConfiguration.java

+    private double loadManagerDefaultMessageRateOut = 50;
+
+    // Name of load manager to use
+    private String loadManagerClassName = "com.yahoo.pulsar.broker.loadbalance.impl.SimpleLoadManagerImpl";


Use SimpleLoadManagerImpl.class.getName(), so that it gets updated if the class is renamed.

There's a bit of an issue with this, in that pulsar-broker depends on pulsar-broker-common and maven doesn't allow circular dependencies. Without package reorganization I'm not sure it can be done this way.

Ok, no problem then

merlimat · 2017-03-24T04:29:00Z

pulsar-broker/src/main/java/com/yahoo/pulsar/broker/BrokerData.java

+ * ZooKeeper but are maintained by the leader broker (Map<String, BundleData>).
+ */
+public class BrokerData {
+	private LocalBrokerData localData;


Formatting seems a bit off, it should be 4 spaces and no tabs

merlimat · 2017-03-24T04:30:41Z

pulsar-broker/src/main/java/com/yahoo/pulsar/broker/PulsarService.java

@@ -201,6 +208,28 @@ public void close() throws PulsarServerException {
        }
    }

+    private class LoadManagerWatcher implements Watcher {
+        public void process(final WatchedEvent event) {
+            new Thread(() -> {


It's preferable to reuse an existing executor instead of spawing new threads

merlimat · 2017-03-24T04:32:52Z

pulsar-broker/src/main/java/com/yahoo/pulsar/broker/PulsarService.java

+        public void process(final WatchedEvent event) {
+            new Thread(() -> {
+                try {
+                    LOG.info("Attempting to change load manager");


ServiceConfiguration already support "dynamic" variables that can be overridded in ZK. You can register to be notified whenever a particular field is updated. Please check with @rdhabalia for more info.

merlimat · 2017-03-24T04:35:10Z

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

+		try {
+			final ServiceConfiguration conf = pulsar.getConfiguration();
+			final Class<?> loadManagerClass = Class.forName(conf.getLoadManagerClassName());
+			// Assume there is a constructor with one argument of PulsarService.


Instead of assuming a particular constructor, a common practice with interfaces and reflection is to assume default constructor and then enforce an init method in the interface:

void initialize(PulsarService service);

That sounds fine, the only problem I would have with that is that I would lose the ability to finalize many fields, but that probably is not a big deal so I will change it

merlimat · 2017-03-24T04:57:14Z

pulsar-testclient/src/main/java/com/yahoo/pulsar/testclient/LoadSimulationServer.java

+		private Producer getNewProducer() throws Exception {
+			while (true) {
+				try {
+					return client.createProducerAsync(topic, producerConf).get();


this could be just

return client.createProducer(topic, producerConf);

merlimat · 2017-03-24T04:59:27Z

pulsar-testclient/src/main/java/com/yahoo/pulsar/testclient/LoadSimulationServer.java

+				if (!stop.get()) {
+					// The Producer failed due to an exception: attempt to get
+					// another producer.
+					producer = getNewProducer();


Once you have a producer, it's good forever. Even if you get an except, the producer instance is still valid and it will recover by itself.

Hmm, it seems that for whatever reason, I am experiencing issues related to netty/direct memory if I do not attempt to make a new producer:

io.netty.util.IllegalReferenceCountException: refCnt: 0, increment: 1 at io.netty.buffer.AbstractReferenceCountedByteBuf.retain(AbstractReferenceCountedByteBuf.java:63) at com.yahoo.pulsar.common.compression.CompressionCodecNone.encode(CompressionCodecNone.java:27) at com.yahoo.pulsar.client.impl.BatchMessageContainer.getCompressedBatchMetadataAndPayload(BatchMessageContainer.java:107) at com.yahoo.pulsar.client.impl.ProducerImpl.batchMessageAndSend(ProducerImpl.java:1019) at com.yahoo.pulsar.client.impl.ProducerImpl.doBatchSendAndAdd(ProducerImpl.java:293) at com.yahoo.pulsar.client.impl.ProducerImpl.sendAsync(ProducerImpl.java:237) at com.yahoo.pulsar.client.impl.ProducerImpl.sendAsync(ProducerImpl.java:134) at com.yahoo.pulsar.client.impl.ProducerBase.sendAsync(ProducerBase.java:47) at com.yahoo.pulsar.testclient.LoadSimulationServer$TradeUnit.start(LoadSimulationServer.java:112) at com.yahoo.pulsar.testclient.LoadSimulationServer.lambda$handle$5(LoadSimulationServer.java:212) at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511) at java.util.concurrent.FutureTask.run(FutureTask.java:266) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) at io.netty.util.concurrent.DefaultThreadFactory$DefaultRunnableDecorator.run(DefaultThreadFactory.java:144) at java.lang.Thread.run(Thread.java:745)

As a result, I can end up losing a bunch of bundles in the simulation. I will leave the get new producer logic for now and try to find a better solution later

merlimat · 2017-03-24T05:00:50Z

pulsar-testclient/pom.xml

@@ -39,11 +39,26 @@
 			<scope>test</scope>
 		</dependency>
 		<dependency>
+			<groupId>org.apache.zookeeper</groupId>
+			<artifactId>zookeeper</artifactId>
+			<version>3.4.8</version>


Don't specify artifact version outside <dependencyManagement> section in the parent pom. In this case just omit the version.

merlimat · 2017-03-24T05:03:21Z

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

@@ -159,8 +160,7 @@ private ResourceUsage getMemUsage() {
    }

    private boolean isPhysicalNic(Path path) {
-        try {
-            path = Files.isSymbolicLink(path) ? Files.readSymbolicLink(path) : path;
+            path = Files.isSymbolicLink(path) ? path.toAbsolutePath() : path;


Why removing try/catch here?

readSymbolicLink can throw IOException, but since it was changed, there is no possibility of a checked exception being thrown. I could change it to catch Exception instead, though this catches more exceptions than in the original code.

merlimat · 2017-03-24T05:04:04Z

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

+		// select one of them at the end.
+		for (String broker : candidates) {
+			final double score = getScore(loadData.getBrokerData().get(broker), conf);
+			log.info("{} got score {}", broker, score);


Should this line be more like debug?

rdhabalia · 2017-03-24T23:31:29Z

bin/pulsar-perf

+    consume                 Run a consumer
+    simple-monitor          Continuously receive broker data when using SimpleLoadManagerImpl
+    modular-monitor         Continuously receive broker data when using ModularLoadManagerImpl
+    simulation-server       Run a simulation server acting as a Pulsar client


should we name it as simulation-client?

rdhabalia · 2017-03-24T23:36:45Z

pulsar-broker/src/main/java/com/yahoo/pulsar/broker/BrokerData.java

@@ -0,0 +1,52 @@
+package com.yahoo.pulsar.broker;


it will require to have copy-right License here.

I am not sure why this diff is not outdated: the copy-right is present if you look at Files Changed.

Sure, we can see now.

rdhabalia · 2017-03-24T23:50:23Z

pulsar-broker/src/main/java/com/yahoo/pulsar/broker/PulsarService.java

+                    final String newLoadManagerName =
+                            new String(getZkClient().getData(DYNAMIC_LOAD_MANAGER_ZPATH, this, null));
+
+                    config.setLoadManagerClassName(newLoadManagerName);


should we make LoadManagerClassName dynamic as per feature introduce in #186 and we can register the listener so, we may not have to create this class to update the loadbalancer.

rdhabalia · 2017-03-24T23:55:14Z

pulsar-broker/src/main/java/com/yahoo/pulsar/broker/admin/BrokerStats.java

+            if (lm instanceof SimpleLoadManagerImpl) {
+                return ((SimpleLoadManagerImpl) lm).getResourceAvailabilityFor(ns).asMap();
+            } else {
+                return Collections.emptyMap();


what if we just return null rather creating new collections.

rdhabalia · 2017-03-25T00:01:20Z

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

@@ -79,6 +83,11 @@
    void doNamespaceBundleSplit() throws Exception;

    /**
+     * Determine the broker root.
+     */
+    String getBrokerRoot();


should we name it as : getBrokerRootName() as we are returning name rather root node?

rdhabalia · 2017-03-25T00:10:54Z

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

+    }
+
+    // Form a score for a broker using its preallocated bundle data and time
+    // average data.


here again, is it possible to define how do we calculate score?

rdhabalia · 2017-03-25T00:13:28Z

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

-                    // wireless nics don't report speed, ignore them.
-                    return false;
-                }
+        path = Files.isSymbolicLink(path) ? path.toAbsolutePath() : path;


shouldn't we read readSymbolicLink if file isSymbolicLink?

rdhabalia · 2017-03-25T00:16:10Z

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

+import com.yahoo.pulsar.zookeeper.ZooKeeperDataCache;
+
+public class ModularLoadManagerImpl implements ModularLoadManager, ZooKeeperCacheListener<LocalBrokerData> {
+    public static final String LOADBALANCE_BROKERS_ROOT = "/loadbalance/new-brokers";


does it mean we don't write into /loadbalance/brokers anymore once we enable this load-manager? because while migration old broker still tries to get broker's url from path /loadbalance/brokers while redirection of lookup request. and if this node will not be present then lookup may fail?

This was resolved by having NamespaceService call getBrokerRoot instead of using the hardcoded path

EDIT: I see now there is little necessity to separate the two broker roots and have removed getBrokerRoot in favor of simply using /loadbalance/brokers

rdhabalia · 2017-03-25T00:21:24Z

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

+        return broker;
+    }
+
+    private void policyFilter(final ServiceUnitId serviceUnit) {


is the same logic compare to SimpleLoadManagerImpl.getFinalCandidates() to filter out candidate if yes then should we put it into some common place to avoid for any new loadManager?

It is very similar, but at the moment the different in the APIs (Multimap<Long, ResourceUnit> vs. Set<String>) complicate refactoring of this portion of the code significantly.

I had some time today and decided to go ahead and share the code, along with other duplicate code. It makes a somewhat invasive change to SimpleLoadManagerImpl which could possibly impact performance: If there are major concerns there I will revert by to the previous version.

rdhabalia · 2017-03-25T00:29:10Z

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

+
+            final Set<String> oldBundles = lastLoadReport.getBundles();
+            final Set<String> newBundles = loadReport.getBundles();
+            final Set<String> bundleGains = new HashSet<>();


is it possible to move this collection to class level and reuse it? as load-report run every x minutes and it may lil bit contribute in gc?

saandrews · 2017-03-28T19:13:09Z

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

+        // the ranks are bounded. Otherwise (as is the case in LOADBALANCER_STRATEGY_LEAST_MSG, the ranks are simply
+        // the total message rate which is in the range [0,Infinity) so they are unbounded. The
+        // "boundedness" affects how two ranks are compared to see which one is better
+        boolean unboundedRanks = getLoadBalancerPlacementStrategy().equals(LOADBALANCER_STRATEGY_LEAST_MSG);
        synchronized (resourceUnitRankings) {


Since both the methods accessing this map is synchronized, can we remove this synchronization block?

saandrews · 2017-03-28T22:15:30Z

pulsar-broker/src/main/java/com/yahoo/pulsar/broker/BundleData.java

+    // Long term data for this bundle. The time frame of this data is determined
+    // by the number of long term samples
+    // and the bundle update period.
+    private TimeAverageMessageData longTermData;


Since BundleData is only maintained in memory by the leader, we will loose that info when leader restarts. Isn't it useful to have bundle specific historic data preserved across restart?

It is maintained on ZooKeeper also in /loadbalance/broker-data (it is basically the replacement for resource quotas). It is updated to ZooKeeper via writeBundleDataOnZooKeeper.

saandrews · 2017-03-28T23:45:05Z

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

+     */
+    @Override
+    public void onUpdate(final String path, final LocalBrokerData data, final Stat stat) {
+        scheduler.submit(this::updateAll);


If this is invoked when load data is updated by every broker, shouldn't we update the data for that broker instead of all?

You're right, good catch.

EDIT: Actually, the local data is updated via writeBrokerDataOnZooKeeper, which is called periodically by LoadReportUpdaterTask. This updateAll periodic invocation ensures that the load data is up to date; while we could delegate it only to the leader broker, this could complicate things a little bit because:

The load manager does not know directly whether it is the leader broker.

Since it is implemented as a watch we would have to make special stipulations based on whether the broker is the leader broker.

I think that, since updating invokes reads and not writes, it is not so bad for every broker to do it. It is actually similar to SimpleLoadManagerImpl's behavior, since onUpdate updates the rankings for it, for each broker.

saandrews · 2017-03-28T23:48:08Z

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

+     * @return The name of the selected broker, as it appears on ZooKeeper.
+     */
+    @Override
+    public synchronized String selectBrokerForAssignment(final ServiceUnitId serviceUnit) {


Why do we need synchronized block here?

Without the synchronized block, decisions will be made without the latest preallocated data. If many selections are processed within a short time frame, this could result in the least loaded broker receiving too many assignments. A weighted random procedure could counteract this, but it is a little less reliable and more complicated.

rdhabalia · 2017-03-30T00:46:23Z

pulsar-broker-common/src/main/java/com/yahoo/pulsar/broker/ServiceConfiguration.java

@@ -199,6 +199,10 @@
    private boolean loadBalancerEnabled = false;
    // load placement strategy
    private String loadBalancerPlacementStrategy = "weightedRandomSelection"; // weighted random selection
+    // load placement secondary strategy (used to silently test an alternate strategy)
+    private String loadBalancerSecondaryStrategy = null;


do we really need loadBalancerSecondaryStrategy? can't we update loadBalancerPlacementStrategy and dynamically keep changing the strategy?

rdhabalia · 2017-03-30T00:48:06Z

pulsar-broker-common/src/main/java/com/yahoo/pulsar/broker/ServiceConfiguration.java

+
+    // Name of load manager to use
+    @FieldContext(dynamic = true)
+    private String loadManagerClassName = "com.yahoo.pulsar.broker.loadbalance.impl.SimpleLoadManagerImpl";


we should place all the variables in the beginning and then getters/setters.

rdhabalia · 2017-03-30T00:53:29Z

pulsar-broker-common/src/main/java/com/yahoo/pulsar/broker/ServiceConfiguration.java

+    // load placement secondary strategy (used to silently test an alternate strategy)
+    private String loadBalancerSecondaryStrategy = null;
+    // are all bundle placement operations forwarded to a lead broker
+    private boolean loadBalancerIsCentralized = false;


I think we don't need this field. right now, we derive loadBalancerIsCentralized based on strategy. if strategy is leastLoadedServer then loadBalancerIsCentralized will be true else false. So, each LoadBalancer implements isCentralized method.
So, as we already have loadBalancer implementation then LoadBalancerIml.isCentralized() should have predefine value and it shouldn't be dynamic. right?

rdhabalia · 2017-03-30T00:53:57Z

pulsar-broker/src/main/java/com/yahoo/pulsar/broker/BrokerData.java

@@ -0,0 +1,52 @@
+package com.yahoo.pulsar.broker;


Sure, we can see now.

rdhabalia · 2017-03-30T01:06:03Z

...ar-testclient/src/main/java/com/yahoo/pulsar/testclient/ModularLoadManagerBrokerMonitor.java

+            System.out.println("\nBroker Data for " + broker + ":");
+            System.out.println("---------------");
+
+            System.out.println("\nNum Topics: " + localBrokerData.getNumTopics());


can we please replace System.out.print with logger.

Is this strictly necessary? This is meant to feed continuous information in a pretty/human-readable way and the info lines are going to add a lot of clutter.

That's true, though the advantage of using a logger is that you can chose to change levels or save to a file with configuration. Also, every line will get the timestamp printed

rdhabalia · 2017-03-30T01:06:59Z

pulsar-broker/src/main/java/com/yahoo/pulsar/broker/LocalBrokerData.java

+    private Set<String> bundles;
+
+    // The bundles gained since the last invocation of update.
+    private Set<String> lastBundleGains;


do we use lastBundleGains in loadManager?

In is not used in the load manager, but it is used in the monitor to alert users to recent bundle gains.

rdhabalia · 2017-03-30T01:12:43Z

pulsar-broker/src/main/java/com/yahoo/pulsar/broker/BrokerData.java

+ * which are not written to ZooKeeper but are maintained by the leader broker (Map<String, BundleData>).
+ */
+public class BrokerData {
+    private LocalBrokerData localData;


I think LocalBrokerData is a replacement of LoadReport so, can't we add more information into existing LoadReport and use it? In that way we don't have to maintain two classes and any load-manager can deserialize it. is that correct?

While that is true, I think pushing the new fields into LoadReport would add unnecessary clutter to the JSON for users of SimpleLoadManagerImpl and vice-versa for ModularLoadManagerImpl. I think they are different enough to keep separate.

saandrews · 2017-03-30T19:20:48Z

Can we create a separate pull request to document the steps to enable/disable the new load manager?

rdhabalia · 2017-03-30T19:25:54Z

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

-                        / newUsage.directMemory.limit)
-                        : 0;
+                            ? ((newUsage.directMemory.usage - oldUsage.directMemory.usage) * 100
+                                    / newUsage.directMemory.limit)


formatting. spaces

It seems to be formatted correctly according to our formatter

rdhabalia · 2017-03-30T19:27:49Z

pulsar-broker/src/main/java/com/yahoo/pulsar/broker/service/BrokerService.java

+            try {
+                log.info("Attempting to change load manager");
+                final LoadManager newLoadManager = LoadManager.create(pulsar);
+                log.info("Created load manager: {}", className);


should we just have one info statement after creating load-manager.

rdhabalia · 2017-03-30T19:31:37Z

pulsar-broker-common/src/main/java/com/yahoo/pulsar/broker/ServiceConfiguration.java

@@ -199,6 +199,7 @@
    private boolean loadBalancerEnabled = false;
    // load placement strategy
    private String loadBalancerPlacementStrategy = "weightedRandomSelection"; // weighted random selection
+    // load placement secondary strategy (used to silently test an alternate strategy)


I think you missed to remove this line

merlimat · 2017-03-24T22:22:31Z

bin/pulsar-perf

+    produce                 Run a producer
+    consume                 Run a consumer
+    simple-monitor          Continuously receive broker data when using SimpleLoadManagerImpl
+    modular-monitor         Continuously receive broker data when using ModularLoadManagerImpl


Nit-picking: how about calling the 2 commands like :

monitor-simple-load-manager monitor-modular-load-manager

Otherwise the adjective seems to be referred to the "monitor" itself.

merlimat · 2017-03-30T20:03:47Z

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

+            Files.readAllBytes(path.resolve("speed"));
+            return true;
+        } catch (Exception e) {
+            // wireless nics don't report speed, ignore them.


Shouldn't we still be skipping "virtual" NICs?

We decided that, since virtual nics will not be able to resolve speed (?), we may as well let catch block deal with it. That way, we do not need to resolve symbolic links or get absolute paths.

If you're 100% sure that it works, go ahead 😄 . Otherwise just pull back the if-not-virtual check.

saandrews

Good effort to revive the load balancer Brad & Bobbey 👍

merlimat

👍

…vert properly (apache#311) When `KafkaConsumer` send OffsetCommitRequest, the field `retentionTime` is always set to -1, and kafka server convert the retentionTime according to server config. this patch add convert logic the same as kafka server to avoid kop expire the wrong commit offset in `GroupMetadataManager.cleanupGroupMetadata` logic. test in `KafkaRequestHandlerTest.testOffsetCommitRequestRetentionMs`

Introduce new load manager API

1c46f5b

merlimat reviewed Mar 22, 2017

View reviewed changes

bobbeyreese added 8 commits March 22, 2017 14:02

Fix formatting and style issues

bbdf123

Fix imports

1f85e00

Add back renamed classes

713ff38

Add policies to ModularLoadManagerImpl

5e343c9

Make system resource usage more influential

4f97664

Fix reflective setting of load manager in tests

b7ec533

Conform with mockito syntax

81b4725

Merge branch 'master' of https://github.com/yahoo/pulsar

edf6f4e

merlimat assigned bobbeyreese Mar 24, 2017

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

merlimat added this to the 1.17 milestone Mar 24, 2017

merlimat reviewed Mar 24, 2017

View reviewed changes

bobbeyreese added 5 commits March 24, 2017 14:08

Improve formatting and design

925ffca

Revert to old simulation behavior, use reflective interface design

8ae61c1

Remove unnecessary async call

9b5358b

Fix formatting

653a853

Remove git merge remnant

2203768

rdhabalia reviewed Mar 25, 2017

View reviewed changes

bobbeyreese added 4 commits March 27, 2017 11:21

Merge branch 'master' of https://github.com/yahoo/pulsar

db12618

Refactor some classes and put loadManagerClassName in dynamic conf

0d4e74d

Delete LoadSimulationServer

aa2d580

Clean API, share more code

358d1ff

saandrews reviewed Mar 28, 2017

View reviewed changes

bobbeyreese added 4 commits March 28, 2017 16:59

Remove redundant synchronized block, make correction to onUpdate

1ea109e

Fix policy filter bug

31d60cb

Remove availableActiveBrokers.get() call

1410008

Use connection pooling

92028f9

rdhabalia reviewed Mar 30, 2017

View reviewed changes

Remove unused code

ba9c032

rdhabalia reviewed Mar 30, 2017

View reviewed changes

Brad McMillen added 2 commits March 30, 2017 12:58

Don't try reading the symlink and converting to absolute path

348aba3

Merge branch 'master' of https://github.com/bobbeyreese/pulsar

bf79da2

merlimat reviewed Mar 30, 2017

View reviewed changes

bobbeyreese added 2 commits March 30, 2017 13:20

Remove redundant and unnecessary statments

1efe869

Merge branch 'master' of https://github.com/bobbeyreese/pulsar

dbca10a

bobbeyreese mentioned this pull request Mar 30, 2017

Add documentation for changing load manager #325

Merged

Brad McMillen and others added 5 commits March 30, 2017 14:04

Add test back to see if path contains /virtual/

96c0d86

Merge branch 'master' of https://github.com/bobbeyreese/pulsar

67b1fe5

Replace System.out statements with log.info

9e39cc7

Merge branch 'master' of https://github.com/bobbeyreese/pulsar

9b2e59c

Change %s to {}

403ee01

saandrews approved these changes Mar 30, 2017

View reviewed changes

merlimat approved these changes Mar 30, 2017

View reviewed changes

saandrews merged commit 1e02afb into apache:master Mar 30, 2017

bobbeyreese mentioned this pull request Apr 3, 2017

Improve Simulation Tools and Add Documentation #328

Merged

xiaotongwang1 mentioned this pull request Aug 4, 2021

Pulsar 2.7.0+ KOP 2.7.2.x getPartitionedTopicMetadata timeout #11532

Closed

Introduce new load manager API #303

Introduce new load manager API #303

Conversation

bobbeyreese commented Mar 21, 2017

Motivation

Modifications

Result

yahoocla commented Mar 21, 2017

merlimat 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

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

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

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

bobbeyreese Mar 27, 2017 • edited Loading

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 Mar 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobbeyreese Mar 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobbeyreese Mar 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobbeyreese Mar 28, 2017 • edited Loading

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

saandrews commented Mar 30, 2017

Choose a reason for hiding this comment

bobbeyreese Mar 27, 2017 •

edited

Loading

rdhabalia Mar 25, 2017 •

edited

Loading

bobbeyreese Mar 28, 2017 •

edited

Loading

bobbeyreese Mar 28, 2017 •

edited

Loading

bobbeyreese Mar 28, 2017 •

edited

Loading