heesung-sn commented on code in PR #23549:
URL: https://github.com/apache/pulsar/pull/23549#discussion_r1842615497
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -2921,6 +2921,13 @@ public double
getLoadBalancerBandwidthOutResourceWeight() {
)
private boolean loadBalancerSheddingBundlesWithPoliciesEnabled = false;
+ @FieldContext(
+ dynamic = true,
+ category = CATEGORY_LOAD_BALANCER,
+ doc = "The namespaces skip for load shedding"
Review Comment:
nit: "The namespaces to be excluded from load shedding"
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/ExtensibleLoadManagerImpl.java:
##########
@@ -636,6 +640,15 @@ public CompletableFuture<Optional<String>>
selectAsync(ServiceUnitId bundle,
return Optional.empty();
}
Set<String> candidateBrokers =
availableBrokerCandidates.keySet();
+ Set<String> sheddingExcludedNamespaces =
conf.getLoadBalancerSheddingExcludedNamespaces();
Review Comment:
nit: please add a comment to explain why we use this special treatment here.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/scheduler/TransferShedder.java:
##########
@@ -500,6 +501,13 @@ public Set<UnloadDecision>
findBundlesForUnloading(LoadManagerContext context,
}
continue;
}
+ final String namespaceName =
NamespaceBundle.getBundleNamespace(bundle);
+ if (sheddingExcludedNamespaces.contains(namespaceName)) {
+ if (debugMode) {
+ log.info("Skipping load shedding for namespace
{}", namespaceName);
Review Comment:
can we prefix this string?
log.info(String.format(CANNOT_UNLOAD_BUNDLE_MSG, " Bundle namespace has been
found in sheddingExcludedNamespaces", bundle)
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/impl/ModularLoadManagerImpl.java:
##########
@@ -151,6 +151,8 @@ public class ModularLoadManagerImpl implements
ModularLoadManager {
// Strategy used to determine where new topics should be placed.
private ModularLoadManagerStrategy placementStrategy;
+ private ModularLoadManagerStrategy roundRobinBrokerSelector;
Review Comment:
nit: can we rename this to sheddingExcludedNamespaceSelectionStrategy in
case we use another selection strategy in the future?
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/ExtensibleLoadManagerImpl.java:
##########
@@ -636,6 +640,15 @@ public CompletableFuture<Optional<String>>
selectAsync(ServiceUnitId bundle,
return Optional.empty();
}
Set<String> candidateBrokers =
availableBrokerCandidates.keySet();
+ Set<String> sheddingExcludedNamespaces =
conf.getLoadBalancerSheddingExcludedNamespaces();
+
+ var namespace =
NamespaceBundle.getBundleNamespace(bundle.toString());
+ if (sheddingExcludedNamespaces.contains(namespace)) {
+ if (debug(conf, log)) {
+ log.info("Use round robin broker selector for
{}", bundle);
+ }
+ return
roundRobinBrokerSelectionStrategy.select(candidateBrokers, bundle, context);
+ }
return
getBrokerSelectionStrategy().select(candidateBrokers, bundle, context);
Review Comment:
can we pass `namespace` in `getBrokerSelectionStrategy` and return the
correct strategy based on the namespace?
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -2921,6 +2921,13 @@ public double
getLoadBalancerBandwidthOutResourceWeight() {
)
private boolean loadBalancerSheddingBundlesWithPoliciesEnabled = false;
+ @FieldContext(
+ dynamic = true,
+ category = CATEGORY_LOAD_BALANCER,
+ doc = "The namespaces skip for load shedding"
+ )
+ private Set<String> loadBalancerSheddingExcludedNamespaces = new
TreeSet<>();
Review Comment:
nit: can we use HashSet?
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/ExtensibleLoadManagerImpl.java:
##########
@@ -161,6 +162,8 @@ public class ExtensibleLoadManagerImpl implements
ExtensibleLoadManager, BrokerS
@Getter
private final BrokerSelectionStrategy brokerSelectionStrategy;
+ private final BrokerSelectionStrategy roundRobinBrokerSelectionStrategy;
Review Comment:
nit: can we rename this to `sheddingExcludedNamespaceSelectionStrategy` in
case we use another selection strategy in the future?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
