Copilot commented on code in PR #6201:
URL: https://github.com/apache/shenyu/pull/6201#discussion_r2447105719
##########
shenyu-loadbalancer/src/main/java/org/apache/shenyu/loadbalancer/cache/UpstreamCacheManager.java:
##########
@@ -142,12 +143,44 @@ public void removeByKey(final String key) {
* @param upstreamList the upstream list
*/
public void submit(final String selectorId, final List<Upstream>
upstreamList) {
- List<Upstream> validUpstreamList =
upstreamList.stream().filter(Upstream::isStatus).collect(Collectors.toList());
- List<Upstream> existUpstream = MapUtils.computeIfAbsent(UPSTREAM_MAP,
selectorId, k -> Lists.newArrayList());
- existUpstream.stream().filter(upstream ->
!validUpstreamList.contains(upstream))
- .forEach(upstream -> task.triggerRemoveOne(selectorId,
upstream));
- validUpstreamList.stream().filter(upstream ->
!existUpstream.contains(upstream))
- .forEach(upstream -> task.triggerAddOne(selectorId, upstream));
- UPSTREAM_MAP.put(selectorId, validUpstreamList);
+ List<Upstream> actualUpstreamList = Objects.isNull(upstreamList) ?
Lists.newArrayList() : upstreamList;
+ List<Upstream> validUpstreamList =
actualUpstreamList.stream().filter(Upstream::isStatus).toList();
+ List<Upstream> offlineUpstreamList =
actualUpstreamList.stream().filter(up -> !up.isStatus()).toList();
Review Comment:
The actualUpstreamList is being iterated twice unnecessarily. Consider using
Collectors.partitioningBy() to partition the list into valid and offline
upstreams in a single pass, improving performance.
```suggestion
Map<Boolean, List<Upstream>> partitionedUpstreams =
actualUpstreamList.stream()
.collect(Collectors.partitioningBy(Upstream::isStatus));
List<Upstream> validUpstreamList = partitionedUpstreams.get(true);
List<Upstream> offlineUpstreamList = partitionedUpstreams.get(false);
```
##########
shenyu-loadbalancer/src/main/java/org/apache/shenyu/loadbalancer/cache/UpstreamCacheManager.java:
##########
@@ -142,12 +143,44 @@ public void removeByKey(final String key) {
* @param upstreamList the upstream list
*/
public void submit(final String selectorId, final List<Upstream>
upstreamList) {
- List<Upstream> validUpstreamList =
upstreamList.stream().filter(Upstream::isStatus).collect(Collectors.toList());
- List<Upstream> existUpstream = MapUtils.computeIfAbsent(UPSTREAM_MAP,
selectorId, k -> Lists.newArrayList());
- existUpstream.stream().filter(upstream ->
!validUpstreamList.contains(upstream))
- .forEach(upstream -> task.triggerRemoveOne(selectorId,
upstream));
- validUpstreamList.stream().filter(upstream ->
!existUpstream.contains(upstream))
- .forEach(upstream -> task.triggerAddOne(selectorId, upstream));
- UPSTREAM_MAP.put(selectorId, validUpstreamList);
+ List<Upstream> actualUpstreamList = Objects.isNull(upstreamList) ?
Lists.newArrayList() : upstreamList;
+ List<Upstream> validUpstreamList =
actualUpstreamList.stream().filter(Upstream::isStatus).toList();
+ List<Upstream> offlineUpstreamList =
actualUpstreamList.stream().filter(up -> !up.isStatus()).toList();
+ List<Upstream> existUpstreamList =
MapUtils.computeIfAbsent(UPSTREAM_MAP, selectorId, k -> Lists.newArrayList());
+
+ if (actualUpstreamList.isEmpty()) {
+ existUpstreamList.forEach(up -> task.triggerRemoveOne(selectorId,
up));
+ }
+
+ offlineUpstreamList.forEach(offlineUp -> {
+ existUpstreamList.stream()
+ .filter(existUp -> existUp.equals(offlineUp))
+ .findFirst()
+ .ifPresent(up -> task.triggerRemoveOne(selectorId, up));
Review Comment:
This nested loop results in O(n*m) complexity. Convert existUpstreamList to
a Set or Map before the forEach loop to achieve O(n+m) complexity when matching
offline upstreams.
```suggestion
// Use a Set for O(1) lookups instead of nested loops
java.util.Set<Upstream> existUpstreamSet = new
java.util.HashSet<>(existUpstreamList);
offlineUpstreamList.forEach(offlineUp -> {
if (existUpstreamSet.contains(offlineUp)) {
task.triggerRemoveOne(selectorId, offlineUp);
}
```
##########
shenyu-loadbalancer/src/main/java/org/apache/shenyu/loadbalancer/cache/UpstreamCacheManager.java:
##########
@@ -142,12 +143,44 @@ public void removeByKey(final String key) {
* @param upstreamList the upstream list
*/
public void submit(final String selectorId, final List<Upstream>
upstreamList) {
- List<Upstream> validUpstreamList =
upstreamList.stream().filter(Upstream::isStatus).collect(Collectors.toList());
- List<Upstream> existUpstream = MapUtils.computeIfAbsent(UPSTREAM_MAP,
selectorId, k -> Lists.newArrayList());
- existUpstream.stream().filter(upstream ->
!validUpstreamList.contains(upstream))
- .forEach(upstream -> task.triggerRemoveOne(selectorId,
upstream));
- validUpstreamList.stream().filter(upstream ->
!existUpstream.contains(upstream))
- .forEach(upstream -> task.triggerAddOne(selectorId, upstream));
- UPSTREAM_MAP.put(selectorId, validUpstreamList);
+ List<Upstream> actualUpstreamList = Objects.isNull(upstreamList) ?
Lists.newArrayList() : upstreamList;
+ List<Upstream> validUpstreamList =
actualUpstreamList.stream().filter(Upstream::isStatus).toList();
+ List<Upstream> offlineUpstreamList =
actualUpstreamList.stream().filter(up -> !up.isStatus()).toList();
+ List<Upstream> existUpstreamList =
MapUtils.computeIfAbsent(UPSTREAM_MAP, selectorId, k -> Lists.newArrayList());
+
+ if (actualUpstreamList.isEmpty()) {
+ existUpstreamList.forEach(up -> task.triggerRemoveOne(selectorId,
up));
+ }
+
+ offlineUpstreamList.forEach(offlineUp -> {
+ existUpstreamList.stream()
+ .filter(existUp -> existUp.equals(offlineUp))
+ .findFirst()
+ .ifPresent(up -> task.triggerRemoveOne(selectorId, up));
+ });
+
+ if (!validUpstreamList.isEmpty()) {
+ // update upstream weight
+ Map<String, Upstream> existUpstreamMap = existUpstreamList.stream()
+ .collect(Collectors.toMap(this::upstreamMapKey, existUp ->
existUp));
Review Comment:
If existUpstreamList contains duplicate entries (same protocol and URL),
this will throw IllegalStateException due to duplicate keys. Consider using a
merge function like (existing, replacement) -> existing to handle potential
duplicates.
```suggestion
.collect(Collectors.toMap(this::upstreamMapKey, existUp ->
existUp, (existing, replacement) -> existing));
```
--
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]
