[SCB-687] add highway server connection protection #824

zhengyangyong · 2018-07-20T01:23:21Z

Follow this checklist to help us incorporate your contribution quickly and easily:

Make sure there is a JIRA issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
Each commit in the pull request should have a meaningful subject line and body.
Format the pull request title like [SCB-XXX] Fixes bug in ApproximateQuantiles, where you replace SCB-XXX with the appropriate JIRA issue.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Run mvn clean install to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

Add servicecomb.highway.server.connection-limit for setting this limit
Default value is Integer.MAX_VALUE

coveralls · 2018-07-20T01:52:02Z

Coverage decreased (-0.03%) to 87.055% when pulling 2c17acd on zhengyangyong:SCB-687 into 8227f0c on apache:master.

wujimin · 2018-07-20T03:26:06Z

...foundation-vertx/src/main/java/org/apache/servicecomb/foundation/vertx/server/TcpServer.java

@@ -57,8 +69,18 @@ public void init(Vertx vertx, String sslKey, AsyncResultCallback<InetSocketAddre
    }

    netServer.connectHandler(netSocket -> {
-      TcpServerConnection connection = createTcpServerConnection();
-      connection.init(netSocket);
+      if (connectedCounter.get() < connectionLimit) {


seems this is more clear.

if (connectedCounter.incrementAndGet() > connectionLimit){ connectedCounter.decrementAndGet(); netSocket.close(); return; } TcpServerConnection connection = createTcpServerConnection(); connection.init(netSocket, connectedCounter); EventManager.post(new ClientConnectedEvent(netSocket, connectedCount));

wujimin · 2018-07-20T06:28:25Z

...foundation-vertx/src/main/java/org/apache/servicecomb/foundation/vertx/server/TcpServer.java

 import io.vertx.core.Vertx;
 import io.vertx.core.net.NetServer;
 import io.vertx.core.net.NetServerOptions;

 public class TcpServer {
  private URIEndpointObject endpointObject;

+  private final AtomicInteger connectedCounter;


TcpServer will be created for each HighwayServerVerticle instance
so the counter number in server is not correct.

your means I need move it to HighwayServerVerticle instance?

I think we only deploy one HighwayServerVerticle instance (as a spring bean), so may not problem ?

if serivcecomb.highway.thread-count is n, then HighwayServerVerticle instance count is n

sorry, forgotten

wujimin · 2018-07-20T07:00:27Z

integration-tests/spring-pojo-connection-limit-test/src/test/resources/log4j.properties

+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+log4j.rootLogger=INFO, stdout


new projects switch to logback?

may do it in https://issues.apache.org/jira/browse/SCB-683 later

already depend on pojo-test jar, can remove this file

wujimin · 2018-07-20T07:01:04Z

integration-tests/spring-pojo-connection-limit-test/src/test/resources/microservice.yaml

+  highway:
+    address: 0.0.0.0:7070
+    server:
+      connection-limit: 0


consider dynamic update?

I think it is not necessary yet

discuss with customers, we should support this.

already depend on pojo-test jar
and you want to reuse it's declare

so only need to append new config items, no need to declare everything again.

wujimin · 2018-07-20T07:01:57Z

...g-pojo-tests/src/test/java/org/apache/servicecomb/demo/pojo/test/ConnectionEventWatcher.java

+    return counters;
+  }
+
+  @Subscribe


all eventloop thread will sync invoke this
it's not a good sample.

what's this watcher purpose?

only for test

Assert.assertArrayEquals("check connection count change", new Integer[] {1, 0}, watcher.getCounters().toArray());

watcher will receive a connection connected event when consumer calling
event.getTotalConnectedCount() = 1
and when

SCBEngine.getInstance().destroy();

watcher will receive a connection close event
event.getTotalConnectedCount() = 0

Signed-off-by: zhengyangyong <yangyong.zheng@huawei.com>

…pdate Signed-off-by: zhengyangyong <yangyong.zheng@huawei.com>

wujimin · 2018-07-27T01:06:44Z

...oundation-vertx/src/main/java/org/apache/servicecomb/foundation/vertx/ClientClosedEvent.java

@@ -17,6 +17,9 @@

 package org.apache.servicecomb.foundation.vertx;

+/**


indentation is one space?
also need to check other codes.

I copy before, and I am using GoogleStyle in etc

sorry, i made a mistake caused by the char "+"

wujimin · 2018-07-27T01:08:51Z

integration-tests/spring-pojo-connection-limit-test/src/test/resources/microservice.yaml

@@ -17,7 +17,7 @@

 APPLICATION_ID: pojotest-it
 service_description:
-  name: pojo-connection-limit
+  name: pojo


conflict with integration-tests/pojo-test

I think no problem,also reused

PojoService.hello.SayHello("whatever");

in pojo-test but need add servicecomb.highway.server.onnection-limit setting

at last, we will move integration test to real SC, not a mock SC, your logic will cause conflict in that time.

wujimin · 2018-07-27T03:44:16Z

...onnection-limit-test/src/test/java/org/apache/servicecomb/demo/pojo/test/PojoSpringMain.java

+public class PojoSpringMain {
+
+  public static void main(final String[] args) {
+    SpringApplication.run(PojoSpringMain.class, args);


already depend on pojo-test jar
just invoke org.apache.servicecomb.demo.pojo.test.PojoTestMain
no need to depend on complex springboot

Signed-off-by: zhengyangyong <yangyong.zheng@huawei.com>

zhengyangyong requested a review from wujimin July 20, 2018 01:23

wujimin reviewed Jul 20, 2018

View reviewed changes

SCB-687 add highway server connection protection

ab467d8

Signed-off-by: zhengyangyong <yangyong.zheng@huawei.com>

zhengyangyong force-pushed the SCB-687 branch from a923aa3 to c6b670e Compare July 25, 2018 06:51

SCB-687 servicecomb.highway.server.connection-limit support dynamic u…

20fbb7e

…pdate Signed-off-by: zhengyangyong <yangyong.zheng@huawei.com>

zhengyangyong force-pushed the SCB-687 branch from c6b670e to 20fbb7e Compare July 25, 2018 07:28

liubao68 approved these changes Jul 26, 2018

View reviewed changes

wujimin reviewed Jul 27, 2018

View reviewed changes

SCB-687 simplify pojo integration test

a322392

Signed-off-by: zhengyangyong <yangyong.zheng@huawei.com>

liubao68 merged commit 0e8664d into apache:master Aug 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SCB-687] add highway server connection protection #824

[SCB-687] add highway server connection protection #824

zhengyangyong commented Jul 20, 2018 •

edited

Loading

coveralls commented Jul 20, 2018 •

edited

Loading

wujimin Jul 20, 2018

zhengyangyong Jul 20, 2018

wujimin Jul 20, 2018

zhengyangyong Jul 20, 2018

zhengyangyong Jul 20, 2018

wujimin Jul 20, 2018

zhengyangyong Jul 20, 2018

zhengyangyong Jul 20, 2018

wujimin Jul 20, 2018

zhengyangyong Jul 20, 2018 •

edited

Loading

wujimin Jul 27, 2018

wujimin Jul 20, 2018

zhengyangyong Jul 20, 2018

wujimin Jul 22, 2018

zhengyangyong Jul 24, 2018

wujimin Jul 27, 2018

wujimin Jul 20, 2018

wujimin Jul 20, 2018

zhengyangyong Jul 20, 2018

wujimin Jul 27, 2018

zhengyangyong Jul 27, 2018

wujimin Jul 27, 2018

wujimin Jul 27, 2018

zhengyangyong Jul 27, 2018

wujimin Aug 1, 2018

wujimin Jul 27, 2018

zhengyangyong Jul 27, 2018

		@@ -17,6 +17,9 @@

		package org.apache.servicecomb.foundation.vertx;

		/**

[SCB-687] add highway server connection protection #824

[SCB-687] add highway server connection protection #824

Conversation

zhengyangyong commented Jul 20, 2018 • edited Loading

coveralls commented Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhengyangyong Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhengyangyong commented Jul 20, 2018 •

edited

Loading

coveralls commented Jul 20, 2018 •

edited

Loading

zhengyangyong Jul 20, 2018 •

edited

Loading