You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shenyu.apache.org by qi...@apache.org on 2022/03/01 07:39:10 UTC
[incubator-shenyu] branch master updated: [Task #2945] Optimize some code style for shenyu-loadbalance plugin (#2953)
This is an automated email from the ASF dual-hosted git repository.
qicz pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-shenyu.git
The following commit(s) were added to refs/heads/master by this push:
new a3ea0d3 [Task #2945] Optimize some code style for shenyu-loadbalance plugin (#2953)
a3ea0d3 is described below
commit a3ea0d3cfd16bc3530d24f401da6d634dbcbff27
Author: AhahaGe <ah...@163.com>
AuthorDate: Tue Mar 1 15:39:05 2022 +0800
[Task #2945] Optimize some code style for shenyu-loadbalance plugin (#2953)
* 1. comment style from /** * */ to // in method
/** * weight changed. */
2. variable definition like selectedWRR, rename divideUpstream to upstream
3. reimport
4. add necessary comment to make code more readable
5. use enum to replace magic word
* 1. comment style from /** * */ to // in method
/** * weight changed. */
2. variable definition like selectedWRR, rename divideUpstream to upstream
3. reimport
4. add necessary comment to make code more readable
5. use enum to replace magic word
* fix ci error
* PluginMapperTest pluginName System.nanoTime()
---
.../shenyu/admin/mapper/PluginMapperTest.java | 2 +-
.../loadbalancer/factory/LoadBalancerFactory.java | 7 ++---
.../loadbalancer/spi/AbstractLoadBalancer.java | 12 ++++----
.../shenyu/loadbalancer/spi/HashLoadBalancer.java | 25 ++++++++++------
.../shenyu/loadbalancer/spi/LoadBalancer.java | 4 +--
.../loadbalancer/spi/RandomLoadBalancer.java | 34 +++++++---------------
.../loadbalancer/spi/RoundRobinLoadBalancer.java | 22 +++++---------
.../loadbalancer/cache/UpstreamCheckTaskTest.java | 33 +++++++--------------
.../factory/LoadBalancerFactoryTest.java | 12 ++++----
9 files changed, 63 insertions(+), 88 deletions(-)
diff --git a/shenyu-admin/src/test/java/org/apache/shenyu/admin/mapper/PluginMapperTest.java b/shenyu-admin/src/test/java/org/apache/shenyu/admin/mapper/PluginMapperTest.java
index 6e6a067..4059e2d 100644
--- a/shenyu-admin/src/test/java/org/apache/shenyu/admin/mapper/PluginMapperTest.java
+++ b/shenyu-admin/src/test/java/org/apache/shenyu/admin/mapper/PluginMapperTest.java
@@ -241,7 +241,7 @@ public final class PluginMapperTest extends AbstractSpringIntegrationTest {
pluginDTO.setEnabled(true);
pluginDTO.setConfig("test-config");
pluginDTO.setRole("1");
- pluginDTO.setName("test-name" + System.currentTimeMillis());
+ pluginDTO.setName("test-name" + System.nanoTime());
return pluginDTO;
}
}
diff --git a/shenyu-loadbalancer/src/main/java/org/apache/shenyu/loadbalancer/factory/LoadBalancerFactory.java b/shenyu-loadbalancer/src/main/java/org/apache/shenyu/loadbalancer/factory/LoadBalancerFactory.java
index 08a4e0f..2e7f21a 100644
--- a/shenyu-loadbalancer/src/main/java/org/apache/shenyu/loadbalancer/factory/LoadBalancerFactory.java
+++ b/shenyu-loadbalancer/src/main/java/org/apache/shenyu/loadbalancer/factory/LoadBalancerFactory.java
@@ -17,24 +17,23 @@
package org.apache.shenyu.loadbalancer.factory;
+import java.util.List;
import org.apache.shenyu.loadbalancer.entity.Upstream;
import org.apache.shenyu.loadbalancer.spi.LoadBalancer;
import org.apache.shenyu.spi.ExtensionLoader;
-import java.util.List;
-
/**
* The type Load balance Factory.
*/
public class LoadBalancerFactory {
/**
- * Selector divide upstream.
+ * Selector upstream.
*
* @param upstreamList the upstream list
* @param algorithm the loadBalance algorithm
* @param ip the ip
- * @return the divide upstream
+ * @return the upstream
*/
public static Upstream selector(final List<Upstream> upstreamList, final String algorithm, final String ip) {
LoadBalancer loadBalance = ExtensionLoader.getExtensionLoader(LoadBalancer.class).getJoin(algorithm);
diff --git a/shenyu-loadbalancer/src/main/java/org/apache/shenyu/loadbalancer/spi/AbstractLoadBalancer.java b/shenyu-loadbalancer/src/main/java/org/apache/shenyu/loadbalancer/spi/AbstractLoadBalancer.java
index b5ab0f7..070960c 100644
--- a/shenyu-loadbalancer/src/main/java/org/apache/shenyu/loadbalancer/spi/AbstractLoadBalancer.java
+++ b/shenyu-loadbalancer/src/main/java/org/apache/shenyu/loadbalancer/spi/AbstractLoadBalancer.java
@@ -17,11 +17,9 @@
package org.apache.shenyu.loadbalancer.spi;
-import org.apache.shenyu.loadbalancer.entity.Upstream;
-import org.apache.commons.collections4.CollectionUtils;
-
import java.util.List;
-import java.util.Objects;
+import org.apache.commons.collections4.CollectionUtils;
+import org.apache.shenyu.loadbalancer.entity.Upstream;
/**
* The type Abstract load balancer.
@@ -29,17 +27,17 @@ import java.util.Objects;
public abstract class AbstractLoadBalancer implements LoadBalancer {
/**
- * Do select divide upstream.
+ * Do select upstream.
*
* @param upstreamList the upstream list
* @param ip the ip
- * @return the divide upstream
+ * @return the upstream
*/
protected abstract Upstream doSelect(List<Upstream> upstreamList, String ip);
@Override
public Upstream select(final List<Upstream> upstreamList, final String ip) {
- if (Objects.isNull(upstreamList) || CollectionUtils.isEmpty(upstreamList)) {
+ if (CollectionUtils.isEmpty(upstreamList)) {
return null;
}
if (upstreamList.size() == 1) {
diff --git a/shenyu-loadbalancer/src/main/java/org/apache/shenyu/loadbalancer/spi/HashLoadBalancer.java b/shenyu-loadbalancer/src/main/java/org/apache/shenyu/loadbalancer/spi/HashLoadBalancer.java
index a678c77..4fad9f7 100644
--- a/shenyu-loadbalancer/src/main/java/org/apache/shenyu/loadbalancer/spi/HashLoadBalancer.java
+++ b/shenyu-loadbalancer/src/main/java/org/apache/shenyu/loadbalancer/spi/HashLoadBalancer.java
@@ -17,9 +17,6 @@
package org.apache.shenyu.loadbalancer.spi;
-import org.apache.shenyu.loadbalancer.entity.Upstream;
-import org.apache.shenyu.spi.Join;
-
import java.nio.charset.StandardCharsets;
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
@@ -27,6 +24,8 @@ import java.util.List;
import java.util.SortedMap;
import java.util.concurrent.ConcurrentSkipListMap;
import java.util.stream.IntStream;
+import org.apache.shenyu.loadbalancer.entity.Upstream;
+import org.apache.shenyu.spi.Join;
/**
* hash algorithm impl.
@@ -34,17 +33,25 @@ import java.util.stream.IntStream;
@Join
public class HashLoadBalancer extends AbstractLoadBalancer {
+ /**
+ * virtual node used to solve unbalanced load.
+ */
private static final int VIRTUAL_NODE_NUM = 5;
+ /**
+ * consistent hash with virtual node to select upstream.
+ *
+ * @param upstreamList the upstream list
+ * @param ip the ip
+ * @return selected upstream
+ */
@Override
public Upstream doSelect(final List<Upstream> upstreamList, final String ip) {
final ConcurrentSkipListMap<Long, Upstream> treeMap = new ConcurrentSkipListMap<>();
- upstreamList.stream().forEach(upstream -> {
- IntStream.range(0, VIRTUAL_NODE_NUM).forEach(i -> {
- long addressHash = hash("SHENYU-" + upstream.getUrl() + "-HASH-" + i);
- treeMap.put(addressHash, upstream);
- });
- });
+ upstreamList.forEach(upstream -> IntStream.range(0, VIRTUAL_NODE_NUM).forEach(i -> {
+ long addressHash = hash("SHENYU-" + upstream.getUrl() + "-HASH-" + i);
+ treeMap.put(addressHash, upstream);
+ }));
long hash = hash(ip);
SortedMap<Long, Upstream> lastRing = treeMap.tailMap(hash);
if (!lastRing.isEmpty()) {
diff --git a/shenyu-loadbalancer/src/main/java/org/apache/shenyu/loadbalancer/spi/LoadBalancer.java b/shenyu-loadbalancer/src/main/java/org/apache/shenyu/loadbalancer/spi/LoadBalancer.java
index c75f6d6..4809aa6 100644
--- a/shenyu-loadbalancer/src/main/java/org/apache/shenyu/loadbalancer/spi/LoadBalancer.java
+++ b/shenyu-loadbalancer/src/main/java/org/apache/shenyu/loadbalancer/spi/LoadBalancer.java
@@ -23,7 +23,7 @@ import org.apache.shenyu.spi.SPI;
import java.util.List;
/**
- * LoadBalancer interface spi .
+ * LoadBalancer interface spi.
*/
@SPI
public interface LoadBalancer {
@@ -33,7 +33,7 @@ public interface LoadBalancer {
*
* @param upstreamList upstream list
* @param ip ip
- * @return divide upstream
+ * @return upstream
*/
Upstream select(List<Upstream> upstreamList, String ip);
}
diff --git a/shenyu-loadbalancer/src/main/java/org/apache/shenyu/loadbalancer/spi/RandomLoadBalancer.java b/shenyu-loadbalancer/src/main/java/org/apache/shenyu/loadbalancer/spi/RandomLoadBalancer.java
index 44b7976..12670fb 100644
--- a/shenyu-loadbalancer/src/main/java/org/apache/shenyu/loadbalancer/spi/RandomLoadBalancer.java
+++ b/shenyu-loadbalancer/src/main/java/org/apache/shenyu/loadbalancer/spi/RandomLoadBalancer.java
@@ -38,9 +38,7 @@ public class RandomLoadBalancer extends AbstractLoadBalancer {
if (totalWeight > 0 && !sameWeight) {
return random(totalWeight, upstreamList);
}
- /**
- * If the weights are the same or the weights are 0 then random.
- */
+ // If the weights are the same or the weights are 0 then random.
return random(upstreamList);
}
@@ -50,9 +48,7 @@ public class RandomLoadBalancer extends AbstractLoadBalancer {
for (int i = 0; i < length; i++) {
int weight = getWeight(upstreamList.get(i));
if (i > 0 && weight != getWeight(upstreamList.get(i - 1))) {
- /**
- * Calculate whether the weight of ownership is the same.
- */
+ // Calculate whether the weight of ownership is the same.
sameWeight = false;
break;
}
@@ -61,32 +57,24 @@ public class RandomLoadBalancer extends AbstractLoadBalancer {
}
private int calculateTotalWeight(final List<Upstream> upstreamList) {
- /**
- * total weight.
- */
+ // total weight.
int totalWeight = 0;
- for (Upstream divideUpstream : upstreamList) {
- int weight = getWeight(divideUpstream);
- /**
- * Cumulative total weight.
- */
+ for (Upstream upstream : upstreamList) {
+ int weight = getWeight(upstream);
+ // Cumulative total weight.
totalWeight += weight;
}
return totalWeight;
}
private Upstream random(final int totalWeight, final List<Upstream> upstreamList) {
- /**
- * If the weights are not the same and the weights are greater than 0, then random by the total number of weights.
- */
+ // If the weights are not the same and the weights are greater than 0, then random by the total number of weights.
int offset = RANDOM.nextInt(totalWeight);
- /**
- * Determine which segment the random value falls on
- */
- for (Upstream divideUpstream : upstreamList) {
- offset -= getWeight(divideUpstream);
+ // Determine which segment the random value falls on
+ for (Upstream upstream : upstreamList) {
+ offset -= getWeight(upstream);
if (offset < 0) {
- return divideUpstream;
+ return upstream;
}
}
return upstreamList.get(0);
diff --git a/shenyu-loadbalancer/src/main/java/org/apache/shenyu/loadbalancer/spi/RoundRobinLoadBalancer.java b/shenyu-loadbalancer/src/main/java/org/apache/shenyu/loadbalancer/spi/RoundRobinLoadBalancer.java
index 0d16a1a..b4d62f5 100644
--- a/shenyu-loadbalancer/src/main/java/org/apache/shenyu/loadbalancer/spi/RoundRobinLoadBalancer.java
+++ b/shenyu-loadbalancer/src/main/java/org/apache/shenyu/loadbalancer/spi/RoundRobinLoadBalancer.java
@@ -28,7 +28,7 @@ import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicLong;
/**
- * Round robin load balance impl.
+ * Round-robin load balance impl.
*/
@Join
public class RoundRobinLoadBalancer extends AbstractLoadBalancer {
@@ -51,7 +51,7 @@ public class RoundRobinLoadBalancer extends AbstractLoadBalancer {
long maxCurrent = Long.MIN_VALUE;
long now = System.currentTimeMillis();
Upstream selectedInvoker = null;
- WeightedRoundRobin selectedWRR = null;
+ WeightedRoundRobin selectedWeightedRoundRobin = null;
for (Upstream upstream : upstreamList) {
String rKey = upstream.getUrl();
WeightedRoundRobin weightedRoundRobin = map.get(rKey);
@@ -62,9 +62,7 @@ public class RoundRobinLoadBalancer extends AbstractLoadBalancer {
map.putIfAbsent(rKey, weightedRoundRobin);
}
if (weight != weightedRoundRobin.getWeight()) {
- /**
- * weight changed.
- */
+ // weight changed.
weightedRoundRobin.setWeight(weight);
}
long cur = weightedRoundRobin.increaseCurrent();
@@ -72,15 +70,13 @@ public class RoundRobinLoadBalancer extends AbstractLoadBalancer {
if (cur > maxCurrent) {
maxCurrent = cur;
selectedInvoker = upstream;
- selectedWRR = weightedRoundRobin;
+ selectedWeightedRoundRobin = weightedRoundRobin;
}
totalWeight += weight;
}
if (!updateLock.get() && upstreamList.size() != map.size() && updateLock.compareAndSet(false, true)) {
try {
- /**
- * copy -> modify -> update reference.
- */
+ // copy -> modify -> update reference.
ConcurrentMap<String, WeightedRoundRobin> newMap = new ConcurrentHashMap<>(map);
newMap.entrySet().removeIf(item -> now - item.getValue().getLastUpdate() > recyclePeriod);
methodWeightMap.put(key, newMap);
@@ -89,17 +85,15 @@ public class RoundRobinLoadBalancer extends AbstractLoadBalancer {
}
}
if (Objects.nonNull(selectedInvoker)) {
- selectedWRR.sel(totalWeight);
+ selectedWeightedRoundRobin.sel(totalWeight);
return selectedInvoker;
}
- /**
- * should not happen here.
- */
+ // should not happen here.
return upstreamList.get(0);
}
/**
- * The type Weighted round robin.
+ * The type Weighted round-robin.
*/
protected static class WeightedRoundRobin {
diff --git a/shenyu-loadbalancer/src/test/java/org/apache/shenyu/loadbalancer/cache/UpstreamCheckTaskTest.java b/shenyu-loadbalancer/src/test/java/org/apache/shenyu/loadbalancer/cache/UpstreamCheckTaskTest.java
index 8e0c852..d88abfc 100644
--- a/shenyu-loadbalancer/src/test/java/org/apache/shenyu/loadbalancer/cache/UpstreamCheckTaskTest.java
+++ b/shenyu-loadbalancer/src/test/java/org/apache/shenyu/loadbalancer/cache/UpstreamCheckTaskTest.java
@@ -17,14 +17,13 @@
package org.apache.shenyu.loadbalancer.cache;
+import java.util.concurrent.TimeUnit;
import org.apache.shenyu.common.dto.SelectorData;
import org.apache.shenyu.loadbalancer.entity.Upstream;
import org.awaitility.Awaitility;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.Timeout;
-import java.util.concurrent.TimeUnit;
-
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.core.Is.is;
import static org.junit.jupiter.api.Assertions.assertFalse;
@@ -48,9 +47,7 @@ public class UpstreamCheckTaskTest {
@Test
@Timeout(30000)
public void testRun() {
- /**
- * Mock selectorId1~selectorId4 to let it coverage 4 branch of `HealthCheckTask#check` method.
- */
+ // Mock selectorId1~selectorId4 to let it coverage 4 branch of `HealthCheckTask#check` method.
final String selectorId1 = "s1";
SelectorData selectorData1 = mock(SelectorData.class);
final String selectorId2 = "s2";
@@ -65,11 +62,11 @@ public class UpstreamCheckTaskTest {
when(selectorData3.getId()).thenReturn(selectorId3);
when(selectorData4.getId()).thenReturn(selectorId4);
- /**
- * Let it coverage line 165~175
- * We should use powermock or mockito to mock static method of `UpstreamCheckUtils.checkUrl`,
- * But mocked static method is not valid across thread. Because `UpstreamCheckUtils.checkUrl` is called in
- * HealthCheckTask inner thread pool, but mocked in current thread. So we turn to do like below.
+ /*
+ Let it coverage line 165~175
+ We should use powermock or mockito to mock static method of `UpstreamCheckUtils.checkUrl`,
+ But mocked static method is not valid across thread. Because `UpstreamCheckUtils.checkUrl` is called in
+ HealthCheckTask inner thread pool, but mocked in current thread. So we turn to do like below.
*/
when(upstream.getUrl()).thenReturn("");
when(upstream.isHealthy()).thenReturn(true).thenReturn(false);
@@ -79,22 +76,14 @@ public class UpstreamCheckTaskTest {
healthCheckTask.triggerAddOne(selectorData3.getId(), upstream);
healthCheckTask.triggerAddOne(selectorData4.getId(), upstream);
healthCheckTask.schedule();
- /**
- * Wait for the upstream-health-check thread to start.
- */
+ // Wait for the upstream-health-check thread to start.
Awaitility.await().pollDelay(3, TimeUnit.SECONDS).untilAsserted(() -> assertFalse(healthCheckTask.getCheckStarted().get()));
assertTrue(healthCheckTask.getUnhealthyUpstream().get(selectorId1).size() > 0);
- /**
- * Let it coverage line 151~163.
- */
+ // Let it coverage line 151~163.
when(upstream.isHealthy()).thenReturn(false).thenReturn(true);
- /**
- * Even if the address could not connect, it will return false, that mean it will not coverage 151~163.
- */
+ // Even if the address could not connect, it will return false, that mean it will not coverage 151~163.
when(upstream.getUrl()).thenReturn("http://www.baidu.com");
- /**
- * Manually run one time
- */
+ // Manually run one time
healthCheckTask.run();
Awaitility.await().pollDelay(1, TimeUnit.SECONDS).untilAsserted(() -> assertFalse(healthCheckTask.getCheckStarted().get()));
assertTrue(healthCheckTask.getHealthyUpstream().get(selectorId1).size() > 0);
diff --git a/shenyu-loadbalancer/src/test/java/org/apache/shenyu/loadbalancer/factory/LoadBalancerFactoryTest.java b/shenyu-loadbalancer/src/test/java/org/apache/shenyu/loadbalancer/factory/LoadBalancerFactoryTest.java
index e9f49e3..cabe6c9 100644
--- a/shenyu-loadbalancer/src/test/java/org/apache/shenyu/loadbalancer/factory/LoadBalancerFactoryTest.java
+++ b/shenyu-loadbalancer/src/test/java/org/apache/shenyu/loadbalancer/factory/LoadBalancerFactoryTest.java
@@ -17,15 +17,15 @@
package org.apache.shenyu.loadbalancer.factory;
-import org.apache.shenyu.loadbalancer.entity.Upstream;
-import org.junit.jupiter.api.Test;
-
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;
import java.util.stream.IntStream;
import java.util.stream.Stream;
+import org.apache.shenyu.common.enums.LoadBalanceEnum;
+import org.apache.shenyu.loadbalancer.entity.Upstream;
+import org.junit.jupiter.api.Test;
import static org.junit.jupiter.api.Assertions.assertEquals;
@@ -48,7 +48,7 @@ public final class LoadBalancerFactoryTest {
.collect(Collectors.toList());
Map<String, Integer> countMap = new HashMap<>();
IntStream.range(0, 120).forEach(i -> {
- Upstream result = LoadBalancerFactory.selector(upstreamList, "roundRobin", "");
+ Upstream result = LoadBalancerFactory.selector(upstreamList, LoadBalanceEnum.ROUND_ROBIN.getName(), "");
int count = countMap.getOrDefault(result.getUrl(), 0);
countMap.put(result.getUrl(), ++count);
});
@@ -66,7 +66,7 @@ public final class LoadBalancerFactoryTest {
.collect(Collectors.toList());
Map<String, Integer> countMap = new HashMap<>();
IntStream.range(0, 120).forEach(i -> {
- Upstream result = LoadBalancerFactory.selector(upstreamList, "roundRobin", "");
+ Upstream result = LoadBalancerFactory.selector(upstreamList, LoadBalanceEnum.ROUND_ROBIN.getName(), "");
int count = countMap.getOrDefault(result.getUrl(), 0);
countMap.put(result.getUrl(), ++count);
});
@@ -84,7 +84,7 @@ public final class LoadBalancerFactoryTest {
.collect(Collectors.toList());
Map<String, Integer> countMap = new HashMap<>();
IntStream.range(0, 120).forEach(i -> {
- Upstream result = LoadBalancerFactory.selector(upstreamList, "roundRobin", "");
+ Upstream result = LoadBalancerFactory.selector(upstreamList, LoadBalanceEnum.ROUND_ROBIN.getName(), "");
int count = countMap.getOrDefault(result.getUrl(), 0);
countMap.put(result.getUrl(), ++count);
});