From 133fd5c970b50db10db7f35870c94680633f2a25 Mon Sep 17 00:00:00 2001 From: "ken.lj" Date: Wed, 16 Jan 2019 10:48:14 +0800 Subject: [PATCH 1/6] extract compareTo impl to Router interface and concrete Router only provide priority --- .../java/org/apache/dubbo/rpc/cluster/Router.java | 15 +++++++++++++++ .../dubbo/rpc/cluster/router/AbstractRouter.java | 8 -------- .../cluster/router/mock/MockInvokersSelector.java | 11 ++--------- .../rpc/cluster/router/script/ScriptRouter.java | 10 ++++++++++ .../com/alibaba/dubbo/rpc/cluster/Router.java | 7 ------- 5 files changed, 27 insertions(+), 24 deletions(-) diff --git a/dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/Router.java b/dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/Router.java index 5addb8e4911..7067312b1a6 100644 --- a/dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/Router.java +++ b/dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/Router.java @@ -85,4 +85,19 @@ default void notify(List> invokers) { * @return router's priority */ int getPriority(); + + @Override + default int compareTo(Router o) { + if (o == null) { + throw new IllegalArgumentException(); + } + if (this.getPriority() == o.getPriority()) { + if (o.getUrl() == null) { + return -1; + } + return getUrl().toFullString().compareTo(o.getUrl().toFullString()); + } else { + return getPriority() > o.getPriority() ? 1 : -1; + } + } } diff --git a/dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/router/AbstractRouter.java b/dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/router/AbstractRouter.java index da4bd7c37eb..aa47d654519 100644 --- a/dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/router/AbstractRouter.java +++ b/dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/router/AbstractRouter.java @@ -20,9 +20,6 @@ import org.apache.dubbo.configcenter.DynamicConfiguration; import org.apache.dubbo.rpc.cluster.Router; -/** - * TODO Extract more code to here if necessary - */ public abstract class AbstractRouter implements Router { protected int priority; protected boolean force = false; @@ -65,11 +62,6 @@ public void setForce(boolean force) { this.force = force; } - @Override - public int compareTo(Router o) { - return (this.getPriority() >= o.getPriority()) ? 1 : -1; - } - public int getPriority() { return priority; } diff --git a/dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/router/mock/MockInvokersSelector.java b/dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/router/mock/MockInvokersSelector.java index 896638a2c04..d6961d630bd 100644 --- a/dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/router/mock/MockInvokersSelector.java +++ b/dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/router/mock/MockInvokersSelector.java @@ -22,7 +22,6 @@ import org.apache.dubbo.rpc.Invocation; import org.apache.dubbo.rpc.Invoker; import org.apache.dubbo.rpc.RpcException; -import org.apache.dubbo.rpc.cluster.Router; import org.apache.dubbo.rpc.cluster.router.AbstractRouter; import java.util.ArrayList; @@ -95,15 +94,9 @@ private boolean hasMockProviders(final List> invokers) { return hasMockProvider; } - /** - * Always stay on the top of the list - * - * @param o - * @return - */ @Override - public int compareTo(Router o) { - return 1; + public int getPriority() { + return Integer.MAX_VALUE; } } diff --git a/dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/router/script/ScriptRouter.java b/dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/router/script/ScriptRouter.java index 496a0dce122..7d4717d8388 100644 --- a/dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/router/script/ScriptRouter.java +++ b/dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/router/script/ScriptRouter.java @@ -24,6 +24,7 @@ import org.apache.dubbo.rpc.Invoker; import org.apache.dubbo.rpc.RpcContext; import org.apache.dubbo.rpc.RpcException; +import org.apache.dubbo.rpc.cluster.Router; import org.apache.dubbo.rpc.cluster.router.AbstractRouter; import javax.script.Bindings; @@ -120,4 +121,13 @@ public boolean isRuntime() { public boolean isForce() { return url.getParameter(Constants.FORCE_KEY, false); } + + @Override + public int compareTo(Router o) { + if (o == null || o.getClass() != ScriptRouter.class) { + return 1; + } + ScriptRouter c = (ScriptRouter) o; + return this.priority == c.priority ? rule.compareTo(c.rule) : (this.priority > c.priority ? 1 : -1); + } } diff --git a/dubbo-compatible/src/main/java/com/alibaba/dubbo/rpc/cluster/Router.java b/dubbo-compatible/src/main/java/com/alibaba/dubbo/rpc/cluster/Router.java index 2fb30d1bb35..2f19286cf07 100644 --- a/dubbo-compatible/src/main/java/com/alibaba/dubbo/rpc/cluster/Router.java +++ b/dubbo-compatible/src/main/java/com/alibaba/dubbo/rpc/cluster/Router.java @@ -36,8 +36,6 @@ List> route(List List> route(List> invokers, URL url, Invocation invocation) throws RpcException { @@ -63,9 +61,4 @@ default boolean isForce() { default int getPriority() { return 1; } - - @Override - default int compareTo(org.apache.dubbo.rpc.cluster.Router o) { - return compareTo((Router) o); - } } From 9c2c60d672a00adcc3dcf3d1554310bb2c8dd502 Mon Sep 17 00:00:00 2001 From: "ken.lj" Date: Wed, 16 Jan 2019 12:33:38 +0800 Subject: [PATCH 2/6] add override CompareTo back to compatible Router and add UT --- .../com/alibaba/dubbo/rpc/cluster/Router.java | 7 +++ .../dubbo/rpc/cluster/CompatibleRouter.java | 1 + .../dubbo/rpc/cluster/CompatibleRouter2.java | 45 ++++++++++++++++ .../apache/dubbo/rpc/cluster/RouterTest.java | 53 +++++++++++++++++++ 4 files changed, 106 insertions(+) create mode 100644 dubbo-compatible/src/test/java/org/apache/dubbo/rpc/cluster/CompatibleRouter2.java create mode 100644 dubbo-compatible/src/test/java/org/apache/dubbo/rpc/cluster/RouterTest.java diff --git a/dubbo-compatible/src/main/java/com/alibaba/dubbo/rpc/cluster/Router.java b/dubbo-compatible/src/main/java/com/alibaba/dubbo/rpc/cluster/Router.java index 2f19286cf07..6253d4090c5 100644 --- a/dubbo-compatible/src/main/java/com/alibaba/dubbo/rpc/cluster/Router.java +++ b/dubbo-compatible/src/main/java/com/alibaba/dubbo/rpc/cluster/Router.java @@ -36,6 +36,8 @@ List> route(List List> route(List> invokers, URL url, Invocation invocation) throws RpcException { @@ -61,4 +63,9 @@ default boolean isForce() { default int getPriority() { return 1; } + + @Override + default int compareTo(org.apache.dubbo.rpc.cluster.Router o) { + return this.compareTo((Router)o); + } } diff --git a/dubbo-compatible/src/test/java/org/apache/dubbo/rpc/cluster/CompatibleRouter.java b/dubbo-compatible/src/test/java/org/apache/dubbo/rpc/cluster/CompatibleRouter.java index 14f5a0e8ed6..2351b214409 100644 --- a/dubbo-compatible/src/test/java/org/apache/dubbo/rpc/cluster/CompatibleRouter.java +++ b/dubbo-compatible/src/test/java/org/apache/dubbo/rpc/cluster/CompatibleRouter.java @@ -28,6 +28,7 @@ * */ public class CompatibleRouter implements Router { + @Override public URL getUrl() { return null; diff --git a/dubbo-compatible/src/test/java/org/apache/dubbo/rpc/cluster/CompatibleRouter2.java b/dubbo-compatible/src/test/java/org/apache/dubbo/rpc/cluster/CompatibleRouter2.java new file mode 100644 index 00000000000..13350d4572f --- /dev/null +++ b/dubbo-compatible/src/test/java/org/apache/dubbo/rpc/cluster/CompatibleRouter2.java @@ -0,0 +1,45 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.dubbo.rpc.cluster; + +import com.alibaba.dubbo.common.URL; +import com.alibaba.dubbo.rpc.Invocation; +import com.alibaba.dubbo.rpc.Invoker; +import com.alibaba.dubbo.rpc.RpcException; +import com.alibaba.dubbo.rpc.cluster.Router; + +import java.util.List; + +/** + * + */ +public class CompatibleRouter2 implements Router { + @Override + public URL getUrl() { + return null; + } + + @Override + public List> route(List> invokers, URL url, Invocation invocation) throws RpcException { + return null; + } + + @Override + public int compareTo(Router o) { + return 0; + } +} diff --git a/dubbo-compatible/src/test/java/org/apache/dubbo/rpc/cluster/RouterTest.java b/dubbo-compatible/src/test/java/org/apache/dubbo/rpc/cluster/RouterTest.java new file mode 100644 index 00000000000..e8967b00e88 --- /dev/null +++ b/dubbo-compatible/src/test/java/org/apache/dubbo/rpc/cluster/RouterTest.java @@ -0,0 +1,53 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.dubbo.rpc.cluster; + +import com.alibaba.dubbo.rpc.cluster.Router; + +import org.junit.Assert; +import org.junit.BeforeClass; +import org.junit.Test; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; + +/** + * + */ +public class RouterTest { + + private static List routers = new ArrayList<>(); + + @BeforeClass + public static void setUp() { + CompatibleRouter compatibleRouter = new CompatibleRouter(); + routers.add(compatibleRouter); + CompatibleRouter2 compatibleRouter2 = new CompatibleRouter2(); + routers.add(compatibleRouter2); + } + + @Test + public void testCompareTo () { + try { + Collections.sort(routers); + Assert.assertTrue(true); + } catch (Exception e) { + Assert.assertFalse(false); + } + } +} From bf4800bd8b4d88f7ad550d1e23563533dac138d5 Mon Sep 17 00:00:00 2001 From: "ken.lj" Date: Wed, 16 Jan 2019 15:38:24 +0800 Subject: [PATCH 3/6] Fix compareTo in compatible Router --- .../com/alibaba/dubbo/rpc/cluster/Router.java | 8 ++- .../apache/dubbo/rpc/cluster/NewRouter.java | 54 +++++++++++++++++++ .../apache/dubbo/rpc/cluster/RouterTest.java | 6 +-- 3 files changed, 63 insertions(+), 5 deletions(-) create mode 100644 dubbo-compatible/src/test/java/org/apache/dubbo/rpc/cluster/NewRouter.java diff --git a/dubbo-compatible/src/main/java/com/alibaba/dubbo/rpc/cluster/Router.java b/dubbo-compatible/src/main/java/com/alibaba/dubbo/rpc/cluster/Router.java index 6253d4090c5..d8822bcd86e 100644 --- a/dubbo-compatible/src/main/java/com/alibaba/dubbo/rpc/cluster/Router.java +++ b/dubbo-compatible/src/main/java/com/alibaba/dubbo/rpc/cluster/Router.java @@ -26,7 +26,7 @@ import java.util.stream.Collectors; @Deprecated -public interface Router extends org.apache.dubbo.rpc.cluster.Router { +public interface Router extends org.apache.dubbo.rpc.cluster.Router{ @Override com.alibaba.dubbo.common.URL getUrl(); @@ -65,7 +65,11 @@ default int getPriority() { } @Override - default int compareTo(org.apache.dubbo.rpc.cluster.Router o) { + default int compareTo (org.apache.dubbo.rpc.cluster.Router o) { + if (!(o instanceof Router)) { + return 1; + } + return this.compareTo((Router)o); } } diff --git a/dubbo-compatible/src/test/java/org/apache/dubbo/rpc/cluster/NewRouter.java b/dubbo-compatible/src/test/java/org/apache/dubbo/rpc/cluster/NewRouter.java new file mode 100644 index 00000000000..86269062e0a --- /dev/null +++ b/dubbo-compatible/src/test/java/org/apache/dubbo/rpc/cluster/NewRouter.java @@ -0,0 +1,54 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.dubbo.rpc.cluster; + +import org.apache.dubbo.common.URL; +import org.apache.dubbo.rpc.Invocation; +import org.apache.dubbo.rpc.Invoker; +import org.apache.dubbo.rpc.RpcException; + +import java.util.List; + +/** + * + */ +public class NewRouter implements Router { + @Override + public URL getUrl() { + return null; + } + + @Override + public List> route(List> invokers, URL url, Invocation invocation) throws RpcException { + return null; + } + + @Override + public boolean isRuntime() { + return false; + } + + @Override + public boolean isForce() { + return false; + } + + @Override + public int getPriority() { + return 0; + } +} diff --git a/dubbo-compatible/src/test/java/org/apache/dubbo/rpc/cluster/RouterTest.java b/dubbo-compatible/src/test/java/org/apache/dubbo/rpc/cluster/RouterTest.java index e8967b00e88..10c9bcc76ef 100644 --- a/dubbo-compatible/src/test/java/org/apache/dubbo/rpc/cluster/RouterTest.java +++ b/dubbo-compatible/src/test/java/org/apache/dubbo/rpc/cluster/RouterTest.java @@ -16,8 +16,6 @@ */ package org.apache.dubbo.rpc.cluster; -import com.alibaba.dubbo.rpc.cluster.Router; - import org.junit.Assert; import org.junit.BeforeClass; import org.junit.Test; @@ -34,11 +32,13 @@ public class RouterTest { private static List routers = new ArrayList<>(); @BeforeClass - public static void setUp() { + public static void setUp () { CompatibleRouter compatibleRouter = new CompatibleRouter(); routers.add(compatibleRouter); CompatibleRouter2 compatibleRouter2 = new CompatibleRouter2(); routers.add(compatibleRouter2); + NewRouter newRouter = new NewRouter(); + routers.add(newRouter); } @Test From 82c1bcc50411dad920a7ebd36b21464698d61029 Mon Sep 17 00:00:00 2001 From: "ken.lj" Date: Wed, 16 Jan 2019 19:02:16 +0800 Subject: [PATCH 4/6] Fix possible NPE in Router --- .../src/main/java/org/apache/dubbo/rpc/cluster/Router.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/Router.java b/dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/Router.java index 7067312b1a6..af1f75f33f6 100644 --- a/dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/Router.java +++ b/dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/Router.java @@ -95,6 +95,9 @@ default int compareTo(Router o) { if (o.getUrl() == null) { return -1; } + if (getUrl() == null) { + return 1; + } return getUrl().toFullString().compareTo(o.getUrl().toFullString()); } else { return getPriority() > o.getPriority() ? 1 : -1; From d154e1147258a853f8bca1d655f9d5c9ac2b1604 Mon Sep 17 00:00:00 2001 From: "ken.lj" Date: Wed, 16 Jan 2019 19:02:16 +0800 Subject: [PATCH 5/6] Fix possible NPE in Router --- .../src/main/java/org/apache/dubbo/rpc/cluster/Router.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/Router.java b/dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/Router.java index af1f75f33f6..e36a522fb48 100644 --- a/dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/Router.java +++ b/dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/Router.java @@ -93,6 +93,9 @@ default int compareTo(Router o) { } if (this.getPriority() == o.getPriority()) { if (o.getUrl() == null) { + return 1; + } + if (getUrl() == null) { return -1; } if (getUrl() == null) { From f65530c4f47b552ed1b00e0661f39e5237636ab5 Mon Sep 17 00:00:00 2001 From: "ken.lj" Date: Wed, 16 Jan 2019 19:09:34 +0800 Subject: [PATCH 6/6] Change router order in case of getUrl empty --- .../src/main/java/org/apache/dubbo/rpc/cluster/Router.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/Router.java b/dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/Router.java index e36a522fb48..70fd8faa3a6 100644 --- a/dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/Router.java +++ b/dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/Router.java @@ -98,9 +98,6 @@ default int compareTo(Router o) { if (getUrl() == null) { return -1; } - if (getUrl() == null) { - return 1; - } return getUrl().toFullString().compareTo(o.getUrl().toFullString()); } else { return getPriority() > o.getPriority() ? 1 : -1;