[GitHub] [pulsar] eolivelli commented on a diff in pull request #14971: [feature][broker] Add new load manager strategy for load manager to select broker having load below average.

[GitBox <gi...@apache.org>]

eolivelli commented on code in PR #14971:
URL: https://github.com/apache/pulsar/pull/14971#discussion_r867355251


##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/ModularLoadManagerStrategy.java:
##########
@@ -55,11 +58,17 @@ Optional<String> selectBroker(Set<String> candidates, BundleData bundleToAssign,
      */
     static ModularLoadManagerStrategy create(final ServiceConfiguration conf) {
         try {
-            // Only one strategy at the moment.
-            return new LeastLongTermMessageRate(conf);
+            Class<?> loadManagerStrategyClass = Class.forName(conf.getLoadBalancerLoadManagerStrategy());
+            Object loadManagerStrategyInstance = loadManagerStrategyClass.newInstance();
+            if (loadManagerStrategyInstance instanceof ModularLoadManagerStrategy) {
+                return (ModularLoadManagerStrategy) loadManagerStrategyInstance;
+            } else {
+                LOG.error("create load manager strategy failed. using LeastLongTermMessageRate instead.");

Review Comment:
   please throw a error, otherwise the system will behave in a way that is unespected



##########
conf/broker.conf:
##########
@@ -1144,6 +1144,10 @@ defaultNamespaceBundleSplitAlgorithm=range_equally_divide
 # load shedding strategy, support OverloadShedder and ThresholdShedder, default is ThresholdShedder since 2.10.0
 loadBalancerLoadSheddingStrategy=org.apache.pulsar.broker.loadbalance.impl.ThresholdShedder
 
+# load balance load manager strategy, support LeastLongTermMessageRate and ThresholdLoadManagerStrategy,
+# default is LeastLongTermMessageRate
+loadBalancerLoadManagerStrategy=org.apache.pulsar.broker.loadbalance.impl.LeastLongTermMessageRate

Review Comment:
   I wonder if it is better to let the user only configure the classname without the package name, this way we will prevent users to load non-standard classes.
   ```
   loadBalancerLoadManagerStrategy=LeastLongTermMessageRate
   loadBalancerLoadManagerStrategy= ThresholdLoadManagerStrategy
   ```
   
   That would be to add a new API/public extensions point, like Protocol Handlers (and in this case we will require a PIP and a deeper discussion)



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org