This is an automated email from the ASF dual-hosted git repository. liubao pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/servicecomb-java-chassis.git
commit c358424a9a6c27bb7f28cd37440c527e525c7014 Author: GuoYL <[email protected]> AuthorDate: Thu Nov 21 10:51:25 2019 +0800 [SCB-1407] fix bug may cause OOM/use spi instead of autowire/improve unit test --- .../servicecomb/router/cache/RouterRuleCache.java | 36 +++++++++----------- .../router/constom/MicroserviceCache.java | 6 ++-- ...ryInvokeFilter.java => RouterInvokeFilter.java} | 39 ++++++++++++++++------ ...ListFilter.java => RouterServerListFilter.java} | 6 ++-- .../distribute/AbstractRouterDistributor.java | 4 +-- .../router/match/RouterHeaderFilterExt.java | 8 +++++ .../router/match/RouterRuleMatcher.java | 13 +++++--- .../apache/servicecomb/router/model/Matcher.java | 3 +- .../servicecomb/router/model/PolicyRuleItem.java | 1 - .../servicecomb/router/model/ServiceInfoCache.java | 10 ++++++ .../apache/servicecomb/router/model/TagItem.java | 36 ++++++++------------ .../servicecomb/router/RouterDistributorTest.java | 31 ++++++++++------- .../router/VersionCompareUtilTest.java} | 20 ++++++++--- 13 files changed, 126 insertions(+), 87 deletions(-) diff --git a/handlers/handler-router/src/main/java/org/apache/servicecomb/router/cache/RouterRuleCache.java b/handlers/handler-router/src/main/java/org/apache/servicecomb/router/cache/RouterRuleCache.java index 5698357..833955c 100644 --- a/handlers/handler-router/src/main/java/org/apache/servicecomb/router/cache/RouterRuleCache.java +++ b/handlers/handler-router/src/main/java/org/apache/servicecomb/router/cache/RouterRuleCache.java @@ -53,49 +53,43 @@ public class RouterRuleCache { public static boolean doInit(String targetServiceName) { if (!serviceInfoCacheMap.containsKey(targetServiceName)) { synchronized (servicePool.intern(targetServiceName)) { - if (!serviceInfoCacheMap.containsKey(targetServiceName)) { + if (serviceInfoCacheMap.containsKey(targetServiceName)) { return true; } //Yaml not thread-safe - Yaml yaml = new Yaml(); DynamicStringProperty ruleStr = DynamicPropertyFactory.getInstance().getStringProperty( String.format(ROUTE_RULE, targetServiceName), null, () -> { refresh(targetServiceName); DynamicStringProperty tepRuleStr = DynamicPropertyFactory.getInstance() .getStringProperty(String.format(ROUTE_RULE, targetServiceName), null); - addAllRule(targetServiceName, yaml, tepRuleStr); + addAllRule(targetServiceName, tepRuleStr); }); - return addAllRule(targetServiceName, yaml, ruleStr); + return addAllRule(targetServiceName, ruleStr); } } return true; } - private static boolean addAllRule(String targetServiceName, Yaml yaml, - DynamicStringProperty ruleStr) { + private static boolean addAllRule(String targetServiceName, DynamicStringProperty ruleStr) { if (ruleStr.get() == null) { return false; } - List<PolicyRuleItem> policyRuleItemList = Arrays - .asList(yaml.loadAs(ruleStr.get(), PolicyRuleItem[].class)); - if (CollectionUtils.isEmpty(policyRuleItemList)) { - return false; - } + Yaml yaml = new Yaml(); + List<PolicyRuleItem> policyRuleItemList; try { - if (serviceInfoCacheMap.get(targetServiceName) == null) { - serviceInfoCacheMap.put(targetServiceName, new ServiceInfoCache()); - } - serviceInfoCacheMap.get(targetServiceName).setAllrule(policyRuleItemList); - // init tagitem - serviceInfoCacheMap.get(targetServiceName).getAllrule().forEach(a -> - a.getRoute().forEach(b -> b.initTagItem()) - ); - // sort by precedence - serviceInfoCacheMap.get(targetServiceName).sortRule(); + policyRuleItemList = Arrays + .asList(yaml.loadAs(ruleStr.get(), PolicyRuleItem[].class)); } catch (Exception e) { LOGGER.error("route management Serialization failed: {}", e.getMessage()); return false; } + if (CollectionUtils.isEmpty(policyRuleItemList)) { + return false; + } + ServiceInfoCache serviceInfoCache = new ServiceInfoCache(policyRuleItemList); + if (!serviceInfoCacheMap.containsKey(targetServiceName)) { + serviceInfoCacheMap.put(targetServiceName, serviceInfoCache); + } return true; } diff --git a/handlers/handler-router/src/main/java/org/apache/servicecomb/router/constom/MicroserviceCache.java b/handlers/handler-router/src/main/java/org/apache/servicecomb/router/constom/MicroserviceCache.java index 67781a6..e3332dd 100644 --- a/handlers/handler-router/src/main/java/org/apache/servicecomb/router/constom/MicroserviceCache.java +++ b/handlers/handler-router/src/main/java/org/apache/servicecomb/router/constom/MicroserviceCache.java @@ -35,9 +35,9 @@ public final class MicroserviceCache { } public Microservice getService(String serviceId) { - Microservice micorservice = services.computeIfAbsent(serviceId, (k) -> { - return RegistryUtils.getMicroservice(serviceId); - }); + Microservice micorservice = services.computeIfAbsent(serviceId, (k) -> + RegistryUtils.getMicroservice(serviceId) + ); return micorservice; } } diff --git a/handlers/handler-router/src/main/java/org/apache/servicecomb/router/constom/CanaryInvokeFilter.java b/handlers/handler-router/src/main/java/org/apache/servicecomb/router/constom/RouterInvokeFilter.java similarity index 82% rename from handlers/handler-router/src/main/java/org/apache/servicecomb/router/constom/CanaryInvokeFilter.java rename to handlers/handler-router/src/main/java/org/apache/servicecomb/router/constom/RouterInvokeFilter.java index d130b1f..1e39cc1 100644 --- a/handlers/handler-router/src/main/java/org/apache/servicecomb/router/constom/CanaryInvokeFilter.java +++ b/handlers/handler-router/src/main/java/org/apache/servicecomb/router/constom/RouterInvokeFilter.java @@ -39,9 +39,9 @@ import org.yaml.snakeyaml.Yaml; import com.netflix.config.DynamicPropertyFactory; -public class CanaryInvokeFilter implements HttpServerFilter { +public class RouterInvokeFilter implements HttpServerFilter { - private static final Logger LOGGER = LoggerFactory.getLogger(CanaryInvokeFilter.class); + private static final Logger LOGGER = LoggerFactory.getLogger(RouterInvokeFilter.class); private static final String SERVICECOMB_ROUTER_HEADER = "servicecomb.router.header"; @@ -49,6 +49,11 @@ public class CanaryInvokeFilter implements HttpServerFilter { private static List<String> allHeader = new ArrayList<>(); + /** + * if don't have rule , avoid registered too many callback + */ + private static boolean isFirstTime = true; + @Override public int getOrder() { return -90; @@ -56,7 +61,7 @@ public class CanaryInvokeFilter implements HttpServerFilter { @Override public boolean enabled() { - return false; + return true; } @Override @@ -74,8 +79,7 @@ public class CanaryInvokeFilter implements HttpServerFilter { @Override public Response afterReceiveRequest(Invocation invocation, HttpServletRequestEx httpServletRequestEx) { - loadHeaders(); - if (invocation.getContext(ROUTER_HEADER) != null && !CollectionUtils.isEmpty(allHeader)) { + if (loadHeaders()) { Map<String, String> headerMap = getHeaderMap(httpServletRequestEx); try { invocation.addContext(ROUTER_HEADER, JsonUtils.OBJ_MAPPER.writeValueAsString(headerMap)); @@ -89,23 +93,36 @@ public class CanaryInvokeFilter implements HttpServerFilter { /** * read config and get Header */ - private void loadHeaders() { + private boolean loadHeaders() { + if (!CollectionUtils.isEmpty(allHeader) && !isFirstTime) { + return true; + } + isFirstTime = false; DynamicStringProperty headerStr = DynamicPropertyFactory.getInstance() .getStringProperty(SERVICECOMB_ROUTER_HEADER, null, () -> { - allHeader = null; DynamicStringProperty temHeader = DynamicPropertyFactory.getInstance() .getStringProperty(SERVICECOMB_ROUTER_HEADER, null); - Yaml yaml = new Yaml(); - allHeader = yaml.load(temHeader.get()); + if (!addAllHeaders(temHeader.get())) { + allHeader = new ArrayList<>(); + } }); + return addAllHeaders(headerStr.get()); + } + + private boolean addAllHeaders(String str) { + if (StringUtils.isEmpty(str)) { + return false; + } try { - if (!CollectionUtils.isEmpty(allHeader)) { + if (CollectionUtils.isEmpty(allHeader)) { Yaml yaml = new Yaml(); - allHeader = yaml.load(headerStr.get()); + allHeader = yaml.load(str); } } catch (Exception e) { LOGGER.error("route management Serialization failed: {}", e.getMessage()); + return false; } + return true; } /** diff --git a/handlers/handler-router/src/main/java/org/apache/servicecomb/router/constom/CanaryServerListFilter.java b/handlers/handler-router/src/main/java/org/apache/servicecomb/router/constom/RouterServerListFilter.java similarity index 94% rename from handlers/handler-router/src/main/java/org/apache/servicecomb/router/constom/CanaryServerListFilter.java rename to handlers/handler-router/src/main/java/org/apache/servicecomb/router/constom/RouterServerListFilter.java index 0c8307a..b258b69 100644 --- a/handlers/handler-router/src/main/java/org/apache/servicecomb/router/constom/CanaryServerListFilter.java +++ b/handlers/handler-router/src/main/java/org/apache/servicecomb/router/constom/RouterServerListFilter.java @@ -34,9 +34,9 @@ import org.apache.servicecomb.serviceregistry.api.registry.Microservice; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -public class CanaryServerListFilter implements ServerListFilterExt { +public class RouterServerListFilter implements ServerListFilterExt { - private static final Logger LOGGER = LoggerFactory.getLogger(CanaryServerListFilter.class); + private static final Logger LOGGER = LoggerFactory.getLogger(RouterServerListFilter.class); private static final String ENABLE = "servicecomb.router.type"; @@ -49,7 +49,7 @@ public class CanaryServerListFilter implements ServerListFilterExt { @Override public boolean enabled() { return DynamicPropertyFactory.getInstance().getStringProperty(ENABLE, "").get() - .equals("TYPE_ROUTER"); + .equals(TYPE_ROUTER); } @Override diff --git a/handlers/handler-router/src/main/java/org/apache/servicecomb/router/distribute/AbstractRouterDistributor.java b/handlers/handler-router/src/main/java/org/apache/servicecomb/router/distribute/AbstractRouterDistributor.java index 646374c..a192ec6 100644 --- a/handlers/handler-router/src/main/java/org/apache/servicecomb/router/distribute/AbstractRouterDistributor.java +++ b/handlers/handler-router/src/main/java/org/apache/servicecomb/router/distribute/AbstractRouterDistributor.java @@ -106,10 +106,10 @@ public abstract class AbstractRouterDistributor<T extends Server, E> implements .getVersion(); Map<TagItem, List<T>> versionServerMap = new HashMap<>(); for (T server : list) { - //获得目标服务 + //get server E ms = getIns.apply(server); if (getServerName.apply(ms).equals(serviceName)) { - //最多匹配原则 + //most matching TagItem tagItem = new TagItem(getVersion.apply(ms), getProperties.apply(ms)); TagItem targetTag = null; int maxMatch = 0; diff --git a/handlers/handler-router/src/main/java/org/apache/servicecomb/router/match/RouterHeaderFilterExt.java b/handlers/handler-router/src/main/java/org/apache/servicecomb/router/match/RouterHeaderFilterExt.java index 62e5161..46af635 100644 --- a/handlers/handler-router/src/main/java/org/apache/servicecomb/router/match/RouterHeaderFilterExt.java +++ b/handlers/handler-router/src/main/java/org/apache/servicecomb/router/match/RouterHeaderFilterExt.java @@ -24,5 +24,13 @@ import java.util.Map; **/ public interface RouterHeaderFilterExt { + default int getOrder() { + return 0; + } + + default boolean enabled() { + return true; + } + Map<String, String> doFilter(Map<String, String> invokeHeader); } diff --git a/handlers/handler-router/src/main/java/org/apache/servicecomb/router/match/RouterRuleMatcher.java b/handlers/handler-router/src/main/java/org/apache/servicecomb/router/match/RouterRuleMatcher.java index 7e3303e..7225303 100644 --- a/handlers/handler-router/src/main/java/org/apache/servicecomb/router/match/RouterRuleMatcher.java +++ b/handlers/handler-router/src/main/java/org/apache/servicecomb/router/match/RouterRuleMatcher.java @@ -16,10 +16,11 @@ */ package org.apache.servicecomb.router.match; +import java.util.List; import java.util.Map; +import org.apache.servicecomb.foundation.common.utils.SPIServiceUtils; import org.apache.servicecomb.router.cache.RouterRuleCache; import org.apache.servicecomb.router.model.PolicyRuleItem; -import org.springframework.beans.factory.annotation.Autowired; /** * @Author GuoYl123 @@ -27,12 +28,12 @@ import org.springframework.beans.factory.annotation.Autowired; **/ public class RouterRuleMatcher { - @Autowired(required = false) - private RouterHeaderFilterExt routerHeaderFilterExt; + private List<RouterHeaderFilterExt> filters; private static RouterRuleMatcher instance = new RouterRuleMatcher(); private RouterRuleMatcher() { + this.filters = SPIServiceUtils.loadSortedService(RouterHeaderFilterExt.class); } /** @@ -42,8 +43,10 @@ public class RouterRuleMatcher { * @return */ public PolicyRuleItem match(String serviceName, Map<String, String> invokeHeader) { - if (routerHeaderFilterExt != null) { - invokeHeader = routerHeaderFilterExt.doFilter(invokeHeader); + for (RouterHeaderFilterExt filterExt : filters) { + if (filterExt.enabled()) { + invokeHeader = filterExt.doFilter(invokeHeader); + } } for (PolicyRuleItem rule : RouterRuleCache.getServiceInfoCacheMap().get(serviceName) .getAllrule()) { diff --git a/handlers/handler-router/src/main/java/org/apache/servicecomb/router/model/Matcher.java b/handlers/handler-router/src/main/java/org/apache/servicecomb/router/model/Matcher.java index 3f3819e..7db815d 100644 --- a/handlers/handler-router/src/main/java/org/apache/servicecomb/router/model/Matcher.java +++ b/handlers/handler-router/src/main/java/org/apache/servicecomb/router/model/Matcher.java @@ -17,6 +17,7 @@ package org.apache.servicecomb.router.model; import java.util.Map; +import org.springframework.util.CollectionUtils; /** * @Author GuoYl123 @@ -36,7 +37,7 @@ public class Matcher { } public boolean match(Map<String, String> realHeaders) { - if (headers == null) { + if (CollectionUtils.isEmpty(headers)) { return true; } for (Map.Entry<String, HeaderRule> entry : headers.entrySet()) { diff --git a/handlers/handler-router/src/main/java/org/apache/servicecomb/router/model/PolicyRuleItem.java b/handlers/handler-router/src/main/java/org/apache/servicecomb/router/model/PolicyRuleItem.java index 4d6daaa..b13d599 100644 --- a/handlers/handler-router/src/main/java/org/apache/servicecomb/router/model/PolicyRuleItem.java +++ b/handlers/handler-router/src/main/java/org/apache/servicecomb/router/model/PolicyRuleItem.java @@ -73,7 +73,6 @@ public class PolicyRuleItem implements Comparable<PolicyRuleItem> { weightLess = true; route.add(new RouteItem(100 - sum, latestVersionTag)); } - //Collections.sort(route); } @Override diff --git a/handlers/handler-router/src/main/java/org/apache/servicecomb/router/model/ServiceInfoCache.java b/handlers/handler-router/src/main/java/org/apache/servicecomb/router/model/ServiceInfoCache.java index 5a49261..a0a2952 100644 --- a/handlers/handler-router/src/main/java/org/apache/servicecomb/router/model/ServiceInfoCache.java +++ b/handlers/handler-router/src/main/java/org/apache/servicecomb/router/model/ServiceInfoCache.java @@ -35,6 +35,16 @@ public class ServiceInfoCache { public ServiceInfoCache() { } + public ServiceInfoCache(List<PolicyRuleItem> policyRuleItemList) { + this.setAllrule(policyRuleItemList); + // init tagitem + this.getAllrule().forEach(rule -> + rule.getRoute().forEach(RouteItem::initTagItem) + ); + // sort by precedence + this.sortRule(); + } + public void sortRule() { allrule = allrule.stream().sorted().collect(Collectors.toList()); } diff --git a/handlers/handler-router/src/main/java/org/apache/servicecomb/router/model/TagItem.java b/handlers/handler-router/src/main/java/org/apache/servicecomb/router/model/TagItem.java index 44d18c3..0fc610d 100644 --- a/handlers/handler-router/src/main/java/org/apache/servicecomb/router/model/TagItem.java +++ b/handlers/handler-router/src/main/java/org/apache/servicecomb/router/model/TagItem.java @@ -26,7 +26,10 @@ import java.util.Objects; **/ public class TagItem { + private static final String VERSION = "version"; + private String version; + private Map<String, String> param; public TagItem() { @@ -40,13 +43,13 @@ public class TagItem { public TagItem(String version) { this.version = version; Map<String, String> param = new HashMap<>(); - param.put("version", version); + param.put(VERSION, version); this.param = param; } public TagItem(Map<String, String> param) { - if (param.containsKey("version")) { - this.version = param.get("version"); + if (param.containsKey(VERSION)) { + this.version = param.get(VERSION); } this.param = param; } @@ -69,35 +72,22 @@ public class TagItem { @Override public int hashCode() { - int result = Objects.hash(version); - if (param != null) { - result = 31 * result + param.hashCode(); - } - return result; + return Objects.hash(getVersion(), getParam()); } @Override - public boolean equals(Object obj) { - if (this == obj) { + public boolean equals(Object o) { + if (this == o) { return true; } - if (obj != null && !(obj instanceof TagItem)) { - return false; - } - TagItem item = (TagItem) obj; - if (this.param == null && item.getParam() == null) { - return this.version.equals(item.getVersion()); - } - if (this.param == null || item.getParam() == null) { + if (!(o instanceof TagItem)) { return false; } - if (this.version != null && item.getVersion() != null) { - return this.version.equals(item.getVersion()) && this.param.equals(item.getParam()); - } - return this.param.equals(item.getParam()); + TagItem tagItem = (TagItem) o; + return Objects.equals(getVersion(), tagItem.getVersion()) && + Objects.equals(getParam(), tagItem.getParam()); } - /** * return match num * diff --git a/handlers/handler-router/src/test/java/org/apache/servicecomb/router/RouterDistributorTest.java b/handlers/handler-router/src/test/java/org/apache/servicecomb/router/RouterDistributorTest.java index d346a9a..89e34e1 100644 --- a/handlers/handler-router/src/test/java/org/apache/servicecomb/router/RouterDistributorTest.java +++ b/handlers/handler-router/src/test/java/org/apache/servicecomb/router/RouterDistributorTest.java @@ -42,11 +42,11 @@ public class RouterDistributorTest { + " match: #匹配策略\n" + " source: xx #匹配某个服务名\n" + " headers: #header匹配\n" - + " xx:\n" - + " regex: xx\n" + + " appId:\n" + + " regex: 01\n" + " caseInsensitive: false # 是否区分大小写，默认为false，区分大小写\n" - + " xxx:\n" - + " exact: xx\n" + + " userId:\n" + + " exact: 01\n" + " route: #路由规则\n" + " - weight: 50\n" + " tags:\n" @@ -55,11 +55,11 @@ public class RouterDistributorTest { + " match:\n" + " source: 1 #匹配某个服务名\n" + " headers: #header匹配\n" - + " xx:\n" - + " regex: xx\n" + + " appId:\n" + + " regex: 01\n" + " caseInsensitive: false # 是否区分大小写，默认为false，区分大小写\n" - + " xxx:\n" - + " exact: xxx\n" + + " userId:\n" + + " exact: 02\n" + " route:\n" + " - weight: 1\n" + " tags:\n" @@ -69,10 +69,17 @@ public class RouterDistributorTest { private static String targetServiceName = "test_server"; @Test + public void testHeaderIsNull() { + List<ServiceIns> list = getMockList(); + List<ServiceIns> serverList = mainFilter(list, null); + Assert.assertEquals(2, serverList.size()); + } + + @Test public void testVersionNotMatch() { Map<String, String> headermap = new HashMap<>(); - headermap.put("xxx", "xx"); - headermap.put("xx", "xx"); + headermap.put("userId", "01"); + headermap.put("appId", "01"); headermap.put("formate", "json"); List<ServiceIns> list = getMockList(); list.remove(1); @@ -84,8 +91,8 @@ public class RouterDistributorTest { @Test public void testVersionMatch() { Map<String, String> headermap = new HashMap<>(); - headermap.put("xxx", "xx"); - headermap.put("xx", "xx"); + headermap.put("userId", "01"); + headermap.put("appId", "01"); headermap.put("formate", "json"); List<ServiceIns> serverList = mainFilter(getMockList(), headermap); Assert.assertEquals(1, serverList.size()); diff --git a/handlers/handler-router/src/main/java/org/apache/servicecomb/router/match/RouterHeaderFilterExt.java b/handlers/handler-router/src/test/java/org/apache/servicecomb/router/VersionCompareUtilTest.java similarity index 53% copy from handlers/handler-router/src/main/java/org/apache/servicecomb/router/match/RouterHeaderFilterExt.java copy to handlers/handler-router/src/test/java/org/apache/servicecomb/router/VersionCompareUtilTest.java index 62e5161..a760bc8 100644 --- a/handlers/handler-router/src/main/java/org/apache/servicecomb/router/match/RouterHeaderFilterExt.java +++ b/handlers/handler-router/src/test/java/org/apache/servicecomb/router/VersionCompareUtilTest.java @@ -14,15 +14,25 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package org.apache.servicecomb.router.match; +package org.apache.servicecomb.router; -import java.util.Map; +import org.apache.servicecomb.router.util.VersionCompareUtil; +import org.junit.Assert; +import org.junit.Test; /** * @Author GuoYl123 - * @Date 2019/10/17 + * @Date 2019/11/21 **/ -public interface RouterHeaderFilterExt { +public class VersionCompareUtilTest { - Map<String, String> doFilter(Map<String, String> invokeHeader); + @Test + public void testVersion() { + Assert.assertTrue(VersionCompareUtil.compareVersion("0.0.1", "0.0.0") > 0); + Assert.assertEquals(0, VersionCompareUtil.compareVersion("0.0.0", "0.0.0")); + Assert.assertTrue(VersionCompareUtil.compareVersion("0.0.0", "0.0.1") < 0); + Assert.assertTrue(VersionCompareUtil.compareVersion("0.0.0", "0.0.0.0") < 0); + Assert.assertTrue(VersionCompareUtil.compareVersion("0.0.1", "0.0.0.0") > 0); + Assert.assertTrue(VersionCompareUtil.compareVersion("0.0.1", "0.0.0.0") > 0); + } }
