spring-cloud
Enable config ServiceInstance weight and provide WeightedLoadBalancer
@OlgaMaciaszek Hello, I am resubmitting a new PR from #1103 , which all commits cherry picked and squashed
Hello @OlgaMaciaszek, comments and checkstyle are fixed, but there is still a points of contention:
Should we add an interface method `getWeight` and use field `weight` to record the weight of `ServiceInstance`, but not use `metadata` directly.
In the earliest version, I had tried use metadata directly. But as we can see, in our current use cases, metadata is never updated, so each time metadata.getOrDefault("weight", "0") called will get same string value, to use it in LoadBalancer we must use int weight = Integer.parseInt(instance.getMetadata().getOrDefault("weight", "0")); to parse to int value, it's slowly and unnecessary.
As for why could user set weight in constructor and setter but not always read from metadata, is considered that LoadBalancer could be static (no service discovery). In this condition, if we request must use metadata to pass weight, the user side code will be complicated and ugly
// Set weight with metadata
var metadata = new HashMap<>();
metadata.put("weight", "100");
// var metadata = Collections.singletonMap("weight", "100");
var instance = new DefaultServiceInstance("instanceId", "serviceId", "localhost", 80, false, metadata);
// Set weight without metadata
var instance = new DefaultServiceInstance("instanceId", "serviceId", "localhost", 80, false, metadata, 100);
We should create a new interface and implementation, that way we don't use metadata and don't pollute the base interface and default impl
We should create a new interface and implementation, that way we don't use metadata and don't pollute the base interface and default impl
We should create a new interface and implementation, that way we don't use metadata and don't pollute the base interface and default impl
Hello, @spencergibb , in the earliest version, I had tried this solution by providing another interface named WeightedServiceInstance, but I don't know what to do when ServiceInstance is not an instance of this interface. Here is the commit link(https://github.com/spring-cloud/spring-cloud-commons/pull/1103/commits/f9ccc10ec9d85df1e312778ec456c6de084350b1).
The DefaultServiceInstance probably should not have any methods regarding weight. Maybe the WeightedLoadBalancer could handle the instances and use some default behaviours when the instance is not a WeightedServiceInstance? Also, please include a proposal defining the instance weight by the users (via properties?) to the PR.
The
DefaultServiceInstanceprobably should not have any methods regarding weight. Maybe theWeightedLoadBalancercould handle the instances and use some default behaviours when the instance is not aWeightedServiceInstance? Also, please include a proposal defining the instance weight by the users (via properties?) to the PR.
Hello, @OlgaMaciaszek. This may not be the main use case for Spring Cloud.
Some traditional enterprises do not have complete cloud infrastructure, including service registration, service discovery and api gateway, or they use RMI-style registry, but do not support RESTful-style service registration (this is what I have experienced). However, when integrating between such systems, it is necessary to avoid single points of failure through load balancing.
Here is an example of the code, I used ConfigurationProperties to autowire from a yml file
@Data
@Component
@ConfigurationProperties(prefix = "service")
public static class ServiceProperties {
private Map<String, List<DefaultServiceInstance>> instances;
}
service:
instances:
hello-world:
- instance-id: instance-1
service-id: hello-world
host: 127.0.0.1
port: 8080
weight: 200 # 2 cores
- instance-id: instance-2
service-id: hello-world
host: 127.0.0.2
port: 8080
weight: 400 # 4 cores
After that we can create a LoadBalancer Bean that gets the name from the environment and the list corresponding to the name from our properties
@Bean
ReactorLoadBalancer<ServiceInstance> weightedLoadBalancer(Environment environment,
ServiceProperties serviceProperties) {
String name = environment.getProperty(LoadBalancerClientFactory.PROPERTY_NAME);
List<ServiceInstance> instances = new ArrayList<>(serviceProperties.instances.get(name));
return new WeightedLoadBalancer(new SimpleObjectProvider<>(new StaticServiceInstanceListSupplier(name, instances)), name);
}
class StaticServiceInstanceListSupplier implements ServiceInstanceListSupplier {
private final String serviceId;
private final List<ServiceInstance> instances;
StaticServiceInstanceListSupplier(String serviceId, List<ServiceInstance> instances) {
this.serviceId = serviceId;
this.instances = instances;
}
@Override
public String getServiceId() {
return serviceId;
}
@Override
public Flux<List<ServiceInstance>> get() {
return Flux.just(instances);
}
}
The
DefaultServiceInstanceprobably should not have any methods regarding weight. Maybe theWeightedLoadBalancercould handle the instances and use some default behaviours when the instance is not aWeightedServiceInstance? Also, please include a proposal defining the instance weight by the users (via properties?) to the PR.
Hello, @OlgaMaciaszek and @spencergibb, I thought about it, because all instances are offered by the same supplier, we can assume that all weighted or unweighted. So we could peek the first element to check whether weighted, if not we could delegate to RoundRobinLoadBalancer to do unweighted choose.
But there is still a special case here, that is, after some custom supplier, the weighted and unweighted are mixed.
Hello, @OlgaMaciaszek and @spencergibb, I thought about it, because all instances are offered by the same supplier, we can assume that all weighted or unweighted. So we could peek the first element to check whether weighted, if not we could delegate to RoundRobinLoadBalancer to do unweighted choose. But there is still a special case here, that is, after some custom supplier, the weighted and unweighted are mixed.
I was thinking we could verify for instances if they are weighted or not if not, converting them to weighted by setting default values in weight. Might be worth measuring the performance impact of this, though.
Here is an example of the code, I used ConfigurationProperties to autowire from a yml file
We'll need to make it usable for all our users before merging, so the PR should include changes that will allow for mapping weight values provided by the users in properties for instances (based on instanceId, but I am thinking based on zone or on other specific metadata value might be useful) without them having to create their own configurations; let me know if you have any idea on how to handle this - if not I could try come up with sth.
Here is an example of the code, I used ConfigurationProperties to autowire from a yml file
We'll need to make it usable for all our users before merging, so the PR should include changes that will allow for mapping
weightvalues provided by the users in properties for instances (based oninstanceId, but I am thinking based on zone or on other specific metadata value might be useful) without them having to create their own configurations; let me know if you have any idea on how to handle this - if not I could try come up with sth.
How to get/set weights from the registry depends on the registry and discovery client implementation, including Spring Cloud Eureka, Spring Cloud Consul and Spring Cloud Kubernetes.
Here is an example that apisix working with Eureka, it uses eureka.instance.metadata-map.weight. As my original issue #1102 , it uses metadata to set weight of the service instance when instance started.
Here is an example of the code, I used ConfigurationProperties to autowire from a yml file
This is not part of this PR. That the code snippet just a simple what user would be when using WeightedLoadBalancer without service registry and discovery client.
This is not part of this PR. That the code snippet just a simple what user would be when using WeightedLoadBalancer without service registry and discovery client.
Yes, and it's something that needs to be implemented (use with SR) before we merge it.
Hello, @OlgaMaciaszek . I have an another idea.
As @spencergibb said, we can provide a WeightedServiceInstance interface and DefaultServiceInstance as default implementation.
And then, maybe we don't need a LoadBalancer to choose the best weight/currentWeight instance, but we could move to WeightedServiceInstanceListSupplier. WeightedServiceInstanceListSupplier has a final field Function<ServiceInstance, Integer> weightFunction, which used to calculate the weight of instance, and if weightFunction is not given, we read the
weight from metadata
private final Function<ServiceInstance, Integer> weightFunction;
public WeightedServiceInstanceListSupplier(ServiceInstanceListSupplier delegate) {
this(delegate, METADATA_WEIGHT_FUNCTION);
}
public WeightedServiceInstanceListSupplier(ServiceInstanceListSupplier delegate, Function<ServiceInstance, Integer> weightFunction) {
super(delegate);
this.weightFunction = weightFunction;
}
After that, weight related is as same as HintBased, ZonePreference and SameInstancePreference, we provide two method in builder withWeighted() and withWeighted(Function<ServiceInstance, Integer> weightFunction) to decide how to map the weight of instance.
Function<ServiceInstance, Integer> weightFunction
To avoid unnecessary boxing and unboxing, it should be ToIntFunction<ServiceInstance> weightFunction
Hello, @OlgaMaciaszek , I submitted some changes, PTAL.
- Split weight related methods to
WeightedServiceInstance. - Remove
AtomicInteger getCurrentWeight()method, usingWeightedLoadBalancer::calculateCurrentWeightinstead. - Allow user setting
ToIntFunction<ServiceInstance> weightFunctionto calculate the weight of instance. If not given, we will check whether instance ofWeightedServiceInstance. At last, we will try to calculate weight frommetadata, if not set inmetadata, it will beDEFAULT_WEIGHT.
Hi @jizhuozhi - thanks a lot. I'm away for a few days - will get back to it next week.
Hi @jizhuozhi - thanks a lot. I'm away for a few days - will get back to it next week.
Hello, @OlgaMaciaszek , no problem, I will continue to think if there is a better solution
Hello @OlgaMaciaszek , I've made two main changes since the last discussion.
- I tried removing the new interface and just using the
weightFunctionto calculate the weight of the instance, this helps keep it as simple as possible - I limit the maximum number of samples each time choose is executed to avoid slowing down the execution of the program due to too many instances.
Here is the optimized microbenchmark, we can see that for very many instances, the performance can remain stable, and the unit test can ensure that the distribution accuracy can reach 99.9% in the case of concurrency.
Benchmark (hostCount) Mode Cnt Score Error Units
WeightedLoadBalancerBenchmarkTests.weightedLoadBalancerChoose 1 thrpt 5 55385.974 ± 14624.403 ops/s
WeightedLoadBalancerBenchmarkTests.weightedLoadBalancerChoose 10 thrpt 5 54463.344 ± 12883.199 ops/s
WeightedLoadBalancerBenchmarkTests.weightedLoadBalancerChoose 100 thrpt 5 42551.335 ± 9318.646 ops/s
WeightedLoadBalancerBenchmarkTests.weightedLoadBalancerChoose 1000 thrpt 5 45279.520 ± 4289.937 ops/s
WeightedLoadBalancerBenchmarkTests.weightedLoadBalancerChoose 10000 thrpt 5 43439.806 ± 9629.446 ops/s
WeightedLoadBalancerBenchmarkTests.weightedLoadBalancerChooseConcurrently 1 thrpt 5 193488.917 ± 17728.115 ops/s
WeightedLoadBalancerBenchmarkTests.weightedLoadBalancerChooseConcurrently 10 thrpt 5 187978.163 ± 43241.231 ops/s
WeightedLoadBalancerBenchmarkTests.weightedLoadBalancerChooseConcurrently 100 thrpt 5 172112.049 ± 51418.301 ops/s
WeightedLoadBalancerBenchmarkTests.weightedLoadBalancerChooseConcurrently 1000 thrpt 5 157070.282 ± 51588.981 ops/s
WeightedLoadBalancerBenchmarkTests.weightedLoadBalancerChooseConcurrently 10000 thrpt 5 160940.176 ± 68303.753 ops/s
Hello @jizhuozhi - sorry for not getting back to you earlier. I think using a WeightedServiceInstanceListSupplier instead of resolving the weight in the LB is a very good idea; could you redraft your PR with this kind of approach? When it comes to passing the weight in the metadata, I understand the pain point of unnecessary parsing, but I'd rather do it and have all the supported SD systems work with this model than not. I agree that instance weight can be final, but I think in order to serve currently supported SD systems (Netflix Eureka, Consul, Zookeeper -not an SD, but also Kubernetes), I thoughut we'd need to get the initial weight value from the metadata. I'm happy to consider a different proposal, but please make sure it would work with all the supported SDs without the need of introducing changes to them.
Hello @jizhuozhi - sorry for not getting back to you earlier.
Hello, @OlgaMaciaszek, don't mention it.
Using a WeightedServiceInstanceListSupplier is my initial idea, but when I implemented it I moved it to the WeightedLoadBalancer. Because if I move the weight calculation logic to another class, then the problem will go back to the origin, how can I make WeightedLoadBalancer get this value without modifying the interface and ensure that users can always complete the configuration with minimal changes.
As for unnecessary parsing, after this period of trying, this may not be a pain point at present, because through micro-benchmarks, we could see the performance overhead caused by this may be minimal. If performance becomes the bottleneck, then I prefer to use a custom weightFunction with cache, which is reserved for enough room for improvement.
How to calculate the weights no longer seems to be the main problem, so now the problem that needs to be solved is how to store this constantly changing current weight. The current implementation is to use ConcurrentHashMap, but this may cause memory leaks, and if weak references are used, it will bring accuracy problems. So it's still debatable.
how can I make WeightedLoadBalancer get this value without modifying the interface and ensure that users can always complete the configuration with minimal changes.
You could verify if it is a WeightedInstance - if not, just use some default weight that could be set by properties.
The current implementation is to use ConcurrentHashMap, but this may cause memory leaks, and if weak references are used, it will bring accuracy problems.
Maybe you could do a quick analysis and some benchmark testing of the solutions you're considering with their drawbacks and advantages, put a summary and then we discuss it here to pick the best one?
You could verify if it is a WeightedInstance - if not, just use some default weight that could be set by properties.
Like this? https://github.com/spring-cloud/spring-cloud-commons/pull/1111/commits/1293960d7752ebc9f37db7836a339c2f99bce383#diff-10d2beb0bc9a6e3eb81e639e73d5c3583c58421faefa6058a1a2a033c87a64f9L132-L140
The reason was described at https://github.com/spring-cloud/spring-cloud-commons/pull/1111#issuecomment-1185483682
Like this? Yes, like this, just making sure to include some kind of default for non-weighted instances.
Like this? Yes, like this, just making sure to include some kind of default for non-weighted instances.
Thanks, I will revert it.
The final point of contention is how to store the current weights of mixed instances after each calculation and ensure that no memory leaks occur. Do you have any good idea?
The final point of contention is how to store the current weights of mixed instances after each calculation and ensure that no memory leaks occur.
Hello, @OlgaMaciaszek , the final solution is to check the type of ServiceInstance.
If ServiceInstance is an instance of WeightedServiceInstance, then use the getCurrentWeight method of the instance directly, otherwise save it in ConcurrentReferenceHashMap (with Collections.synchronizedMap(new) WeakHashMap >()) is equivalent), the accuracy will be sacrificed at this time, but it is guaranteed that there will be no memory leaks.
PTAL, thanks.
@jizhuozhi I'm not sure I understand this comment:
Using a WeightedServiceInstanceListSupplier is my initial idea, but when I implemented it I moved it to the WeightedLoadBalancer. Because if I move the weight calculation logic to another class, then the problem will go back to the origin, how can I make WeightedLoadBalancer get this value without modifying the interface and ensure that users can always complete the configuration with minimal changes.
If the logic was in a WeightedServiceInstanceListSupplier, then there's no need to have a WeightedLoadBalancer. The supplier would provide the list of instances already ordered based on the weight and then any load-balancer would just execute its standard logic. This would remove the need of adding a weighted implementation for each load-balancing algorithm. It is possible I'm missing sth here, though. Please provide a more detailed explanation on why you think that would not work.
Hello @OlgaMaciaszek, thanks for reviewing. Actually there is a concern here, if it is acceptable I will close this PR and resubmit.
For the solution of expanding the list by weight in the supplier, it is a feasible solution, which runs very fast, and can avoid the swarming problem by shuffling the cards.
I'm worried about a very large heap memory usage for large weight lists. Although the occupied space can be reduced by the greatest common divisor, because of formula $n + 1 \equiv 1 \pmod n$, once there are two adjacent numbers, the gcd will be invalid.