nikhiln64 commented on code in PR #6900:
URL: https://github.com/apache/shenyu/pull/6900#discussion_r3744478795
##########
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:
This looks like the right direction, and you are right that the size gate
does not work here. On a single shared map `countMap.size()` is the union of
every selector's domains, so it is not comparable to one selector's
`upstreamList.size()`, and it cannot see a same length member swap, so my
suggestion would have left the cross selector deletion in place. The time based
recycle avoids that cleanly. Refreshing `lastUpdate` for every live domain on
each request means an entry can only be evicted after it has genuinely gone
quiet for `recyclePeriod`, so one selector's cleanup can no longer drop another
selector's live entries and the leak is bounded to about `recyclePeriod`. The
updateLock and the once per period throttle keep the cleanup off the hot path,
and the selected domain is always one you refreshed this call, so the
countMap.get(domain) with the `Objects.nonNull` guard cannot be caught by a
concurrent removeIf. The concurrency reads as sound to me.
The one thing I would think about is the `Long.MIN_VALUE` seed on a revived
entry. If an upstream goes quiet longer than `recyclePeriod` it gets recycled,
and when traffic returns its `ActiveCount` is recreated at `Long.MIN_VALUE`.
Because selection picks the minimum count, that upstream is then chosen on
almost every request until its counter climbs back to its peers, and since the
count is cumulative and never decremented that catch up can take as many
requests as the peers have served in total, so a node that just came back can
absorb nearly all the traffic for a long stretch rather than a brief burst. A
brand new upstream already has this property today, but recycling means an idle
then active upstream hits it too. It might be worth seeding a new or revived
entry to the current minimum of its live peers rather than `Long.MIN_VALUE`, so
it rejoins at parity. Not a blocker, more a behavior to decide on consciously.
On the tests, the stale eviction and multi selector isolation cases you
mentioned are the ones I would want to see. One more worth adding is the revive
case: let an entry age past recyclePeriod so it is recycled, then send traffic
again and assert the distribution does not collapse onto the revived node.
Thanks for turning this around so quickly.
--
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]
