Aias00 commented on code in PR #7161:
URL: https://github.com/apache/shenyu/pull/7161#discussion_r4071711626
##########
shenyu-loadbalancer/src/main/java/org/apache/shenyu/loadbalancer/spi/HashLoadBalancer.java:
##########
@@ -39,33 +43,38 @@ public class HashLoadBalancer extends AbstractLoadBalancer {
* virtual node used to solve unbalanced load.
*/
private static final int VIRTUAL_NODE_NUM = 5;
+
+ private static final ThreadLocal<MessageDigest> MD5 =
ThreadLocal.withInitial(HashLoadBalancer::newMessageDigest);
+
+ private final Map<List<String>, SortedMap<Long, Integer>> ringCache = new
WindowTinyLFUMap<>(Constants.CACHE_MAX_COUNT);
@Override
public Upstream doSelect(final List<Upstream> upstreamList, final
LoadBalanceData data) {
- final ConcurrentSkipListMap<Long, Upstream> treeMap = new
ConcurrentSkipListMap<>();
- upstreamList.forEach(upstream -> IntStream.range(0,
VIRTUAL_NODE_NUM).forEach(i -> {
- long addressHash = hash("SHENYU-" + upstream.getUrl() + "-HASH-" +
i);
- treeMap.put(addressHash, upstream);
- }));
+ final List<String> ringKey = new ArrayList<>(upstreamList.size());
+ upstreamList.forEach(upstream -> ringKey.add(upstream.getUrl()));
+ final SortedMap<Long, Integer> treeMap =
ringCache.computeIfAbsent(List.copyOf(ringKey), this::buildRing);
Review Comment:
Two notes on the cache itself.
**1. `computeIfAbsent` here is a non-atomic check-then-act.**
`WindowTinyLFUMap` extends `AbstractMap` and only overrides
`put/get/remove/clear/size/entrySet`, so this resolves to the default
`Map.computeIfAbsent`:
```java
if ((v = get(key)) == null) {
V newValue;
if ((newValue = mappingFunction.apply(key)) != null) { put(key,
newValue); return newValue; }
}
```
The underlying Caffeine cache makes the individual `get`/`put` safe, so
there is no corruption, but concurrent callers can each build a ring and the
last `put` wins. That is harmless here because `buildRing` is deterministic —
just wasted work on first touch. Worth a comment so nobody later adds something
non-idempotent to `buildRing`.
**2. The bounds are coupled, and 1000 rings is not obviously the right
number.** `Constants.CACHE_MAX_COUNT` is 1000, and each entry holds `upstreams
× VIRTUAL_NODE_NUM` map entries. For a deployment with, say, 200 upstreams
behind one service that is 1000 entries per ring, and up to 1000 distinct
upstream sets retained per load-balancer instance. That is bounded, but it is a
much larger steady-state footprint than the previous "build it and throw it
away" behaviour. Given the ring is keyed on the full URL list, any churn in the
upstream set (a single node flapping) produces a new key each time, so the
working set is driven by how often the upstream list changes.
Neither is a correctness problem — just please make the sizing deliberate
rather than reusing `CACHE_MAX_COUNT` because it was there.
--
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]
