juicewcode commented on code in PR #6900:
URL: https://github.com/apache/shenyu/pull/6900#discussion_r3744347890
##########
shenyu-loadbalancer/src/main/java/org/apache/shenyu/loadbalancer/spi/LeastActiveLoadBalance.java:
##########
@@ -45,6 +45,8 @@ protected Upstream doSelect(final List<Upstream>
upstreamList, final LoadBalance
.filter(key -> !countMap.containsKey(key))
.forEach(domain -> countMap.put(domain, Long.MIN_VALUE));
+ countMap.keySet().retainAll(domainMap.keySet());
Review Comment:
Thanks for the thorough review — you're right, and I've verified each
point against the code.
**Verified.** `LeastActiveLoadBalance` is a bare `@Join`, so `isSingleton`
defaults to `true` and
`ExtensionLoader.getJoin("leastActive")` returns one cached instance
shared by every selector/rule
using leastActive. At the call site, `DividePlugin` fetches the upstream
list per selector
(`UpstreamCacheManager.findUpstreamListBySelectorId(selector.getId())`),
so the same singleton is
invoked with different upstream sets. My unconditional
`retainAll(domainMap.keySet())` therefore
really does evict another selector's live entries on every `doSelect`, and
the lost `computeIfPresent`
increment is real — the original fix traded the leak for cross-selector
count churn.
**On the size-mismatch gate.** I agree with the goal, but I think
comparing sizes runs into a problem
here: `countMap` is a single map shared by all selectors and has no
selector namespacing, so
`countMap.size()` is the union of everyone's domains and is not comparable
to any single selector's
`upstreamList.size()`. In a multi-selector deployment the two sizes almost
never match, so the gate
would stay open and the cross-selector deletion would still happen on
nearly every request. A size
comparison also can't detect a member swap that keeps the list length
unchanged (e.g. A1 replaced by
B1, size still 2), so stale entries could linger. In short, without
namespacing, "the selector's own
size" isn't something we can actually measure — deriving one from the list
would itself break whenever
a selector's first upstream changes.
**Proposed change.** I'd like to keep the single shared map and make
eviction time-based instead,
like `RoundRobinLoadBalancer`'s recycle logic but driven by a timestamp
rather than a size comparison:
```java
@Join
public class LeastActiveLoadBalance extends AbstractLoadBalancer {
private final int recyclePeriod = 60000;
private final ConcurrentMap<String, ActiveCount> countMap = new
ConcurrentHashMap<>(16);
private final AtomicBoolean updateLock = new AtomicBoolean();
private volatile long lastRecycle;
@Override
protected Upstream doSelect(final List<Upstream> upstreamList, final
LoadBalanceData data) {
long now = System.currentTimeMillis();
Map<String, Upstream> domainMap = upstreamList.stream()
.collect(Collectors.toConcurrentMap(Upstream::buildDomain,
upstream -> upstream));
domainMap.keySet().forEach(domain -> {
ActiveCount activeCount = countMap.computeIfAbsent(domain, key
-> new ActiveCount(now));
activeCount.setLastUpdate(now);
});
final String domain = countMap.entrySet().stream()
.filter(entry -> domainMap.containsKey(entry.getKey()))
.min(Comparator.comparingLong(entry ->
entry.getValue().getCount()))
.map(Map.Entry::getKey)
.orElse(upstreamList.get(0).buildDomain());
ActiveCount activeCount = countMap.get(domain);
if (Objects.nonNull(activeCount)) {
activeCount.increase();
}
if (!updateLock.get() && now - lastRecycle > recyclePeriod &&
updateLock.compareAndSet(false, true)) {
try {
countMap.entrySet().removeIf(item -> now -
item.getValue().getLastUpdate() > recyclePeriod);
lastRecycle = now;
} finally {
updateLock.set(false);
}
}
return domainMap.get(domain);
}
protected static class ActiveCount {
private final AtomicLong count = new AtomicLong(Long.MIN_VALUE);
private volatile long lastUpdate;
ActiveCount(final long lastUpdate) {
this.lastUpdate = lastUpdate;
}
void increase() {
count.addAndGet(1);
}
long getCount() {
return count.get();
}
long getLastUpdate() {
return lastUpdate;
}
void setLastUpdate(final long lastUpdate) {
this.lastUpdate = lastUpdate;
}
}
}
```
Key points:
- Live entries are refreshed on every request, so they are never stale and
can't be evicted by
another selector's cleanup — no cross-selector deletion.
- Only entries whose lastUpdate is older than RECYCLE_PERIOD are evicted,
i.e. exactly the
upstreams that were removed from the lists — the leak is bounded to
~`RECYCLE_PERIOD`.
- Eviction is throttled to at most once per RECYCLE_PERIOD and guarded by
an update lock, so steady
state has no per-request cleanup overhead and no concurrent eviction.
I'll update the tests to cover stale-entry eviction and the multi-selector
isolation case. Happy to
adjust RECYCLE_PERIOD or the eviction gating if you'd prefer a different
approach — please let me
know if you see any issues.
--
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]
