[ISSUE#1815]Adding lifecycle interface and resolve the service downtime caused by thread leak #2927

zongtanghu · 2020-05-31T04:24:42Z

Please do not create a Pull Request without creating an issue first.

What is the purpose of the change

The related issue is [ISSUE #1815 ]
(1)Adding lifecycle interface and resolve the service downtime caused by thread leak .

Brief changelog

(1)Define and implement the lifecycle interface and related abstract class codes.
(2)Add lifecycle related codes.
(3)Adjust the ServerHttpAgent and MetricsHttpAgent class to implement the lifecycle interface.

Verifying this change

I have already add the unit test codes.

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

Make sure there is a Github issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a Github issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
Format the pull request title like [ISSUE #123] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Write necessary unit-test to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add integration-test in test module.
Run mvn -B clean package apache-rat:check findbugs:findbugs -Dmaven.test.skip=true to make sure basic checks pass. Run mvn clean install -DskipITs to make sure unit-test pass. Run mvn clean test-compile failsafe:integration-test to make sure integration-test pass.

update 1.3.0 beta

…ome unit test cases.

zongtanghu · 2020-05-31T05:29:25Z

@chuntaojun @yanlinly @wangweizZZ please help to review this pr's codes.

client/src/main/java/com/alibaba/nacos/client/config/http/ServerHttpAgent.java

common/src/main/java/com/alibaba/nacos/common/lifecycle/ResourceLifeCycleManager.java

wangweizZZ · 2020-05-31T05:33:44Z

client/src/main/java/com/alibaba/nacos/client/config/http/MetricsHttpAgent.java

+
+    @Override
+    public void doStart() throws Exception {
+        this.serverHttpAgent.doStart();


use this.serverHttpAgent.start()

wangweizZZ · 2020-05-31T05:37:05Z

client/src/main/java/com/alibaba/nacos/client/config/http/MetricsHttpAgent.java

+
+    @Override
+    public void doStop() throws Exception {
+        this.serverHttpAgent.doStop();


use this.serverHttpAgent.stop()

wangweizZZ · 2020-05-31T06:13:34Z

common/src/main/java/com/alibaba/nacos/common/lifecycle/ResourceLifeCycleManager.java

+    public void register(AbstractLifeCycle instance) {
+        if (!lifeCycleResources.contains(instance)) {
+            synchronized(this) {
+                lockers.put(instance, new Object());


maybe need double check?

Yes, you can see @chuntaojun 's review comments above.

yfh0918 · 2020-06-01T02:59:14Z

提一个建议
ResourceLifeCycleManager中关于lifycycle的注册，可以放到abstractlifycycle的构造函数中进行
或者，载新建个abstractlifycyclewithManager，继承abstractlifycycle做lifycycle的自动注册

zongtanghu · 2020-06-01T03:01:22Z

提一个建议
ResourceLifeCycleManager中关于lifycycle的注册，可以放到abstractlifycycle的构造函数中进行
或者，载新建个abstractlifycyclewithManager，继承abstractlifycycle做lifycycle的自动注册

Okay, good advice, I will optimize this pr's codes.Thanks.

…lass level.

yfh0918 · 2020-06-01T10:41:09Z

漏了一个建议，是不是可以弄个startable和closable，让lifecycle继承它们

zongtanghu · 2020-06-01T10:50:35Z

漏了一个建议，是不是可以弄个startable和closable，让lifecycle继承它们

In last new commit code, I have already defined the closeable interface.

yfh0918 · 2020-06-01T11:11:46Z

Ok节日快乐

…gService class level at the same time.

client/src/main/java/com/alibaba/nacos/client/config/http/ServerHttpAgent.java

client/src/main/java/com/alibaba/nacos/client/config/impl/ClientWorker.java

client/src/main/java/com/alibaba/nacos/client/config/impl/ServerListManager.java

zongtanghu · 2020-06-03T03:23:50Z

I adjust and optimize some codes, please help to review this pr again, thanks. @chuntaojun @yanlinly @wangweizZZ

yfh0918 · 2020-06-03T04:31:44Z

I'm sorry to have made so many suggestions,
The shutdown method is defined in the closeable. Other interfaces, such as configservice, also have the shutdown method. Whether the shutdown can be extends closeable
In addition, there is only destroy method in the lifecycle interface. Can add init method in the lifecycle interface ，the purpose is to reflect the true meaning of lifecycle

zongtanghu · 2020-06-03T04:38:45Z

I'm sorry to have made so many suggestions,
The shutdown method is defined in the closeable. Other interfaces, such as configservice, also have the shutdown method. Whether the shutdown can be extends closeable
In addition, there is only destroy method in the lifecycle interface. Can add init method in the lifecycle interface ，the purpose is to reflect the true meaning of lifecycle

I optimzie original pr's codes, and think it's too complex.Because, Closeable interface is located in detailed implment class level, and lifecycle interface is aimed to be defined and implemented in configservice and namingservice.We put init logical codes in claas constructor method.

yfh0918 · 2020-06-03T04:42:20Z

I'm sorry to have made so many suggestions,
The shutdown method is defined in the closeable. Other interfaces, such as configservice, also have the shutdown method. Whether the shutdown can be extends closeable
In addition, there is only destroy method in the lifecycle interface. Can add init method in the lifecycle interface ，the purpose is to reflect the true meaning of lifecycle

I optimzie original pr's codes, and think it's too complex.Because, Closeable interface is located in detailed implment class level, and lifecycle interface is aimed to be defined and implemented in configservice and namingservice.We put init logical codes in claas constructor method.

This PR is really complicated, and it's a big change. I'm a Virgo. Maybe I'm a little picky. Smile

KomachiSion

I have some suggestion for this PR.

I think there no need to add this when using member normally. It's ok for used this in the set method and constructor uniformly.
Why add destroyXXXService in factory? I think that it's enough for calling shutdown or destory method directly. It is same as call factory.destoryXXXService(XXXService)

api/src/main/java/com/alibaba/nacos/api/config/ConfigFactory.java

api/src/main/java/com/alibaba/nacos/api/naming/NamingFactory.java

client/src/main/java/com/alibaba/nacos/client/config/http/ServerHttpAgent.java

zongtanghu · 2020-06-03T06:45:54Z

I have some suggestion for this PR.

I think there no need to add this when using member normally. It's ok for used this in the set method and constructor uniformly.

Why add destroyXXXService in factory? I think that it's enough for calling shutdown or destory method directly. It is same as call factory.destoryXXXService(XXXService)

My answer:
(1)"this" keywords aims to keep strong unified coding style.
(2)The destroyXXXService in factory can provide user a direct method call to destroy specific configservice/namingservice exhibitly.

… into feature_lifecycle

…d set method in the class and fix some unit test cases.

KomachiSion

It seems no large issues. Just one unused import.

api/src/main/java/com/alibaba/nacos/api/naming/NamingFactory.java

chuntaojun · 2020-06-09T04:36:17Z

client/src/main/java/com/alibaba/nacos/client/naming/core/HostReactor.java

 import com.alibaba.nacos.common.utils.JacksonUtils;

+import com.alibaba.nacos.common.utils.ThreadUtils;
 import org.apache.commons.lang3.StringUtils;

 import java.util.*;


don't import *

…st test cases.

api/src/main/java/com/alibaba/nacos/api/exception/NacosException.java

client/src/main/java/com/alibaba/nacos/client/config/http/HttpAgent.java

yanlinly · 2020-06-10T11:57:37Z

client/src/main/java/com/alibaba/nacos/client/config/impl/ClientWorker.java

@@ -482,12 +499,21 @@ public void run() {

    private void init(Properties properties) {

-        timeout = Math.max(NumberUtils.toInt(properties.getProperty(PropertyKeyConst.CONFIG_LONG_POLL_TIMEOUT),
+        this.timeout = Math.max(ConvertUtils.toInt(properties.getProperty(PropertyKeyConst.CONFIG_LONG_POLL_TIMEOUT),


ConvertUtils 这块为什么要改呢

再下一次提交中修改回原来的NumberUtils

common/src/main/java/com/alibaba/nacos/common/lifecycle/LifeCycle.java

…ated codes.

client/src/main/java/com/alibaba/nacos/client/config/impl/ServerListManager.java

client/src/main/java/com/alibaba/nacos/client/naming/backups/FailoverReactor.java

yanlinly

ok

yanlinly and others added 6 commits May 25, 2020 14:19

Merge pull request alibaba#2875 from alibaba/develop

0ddbe7d

update 1.3.0 beta

[alibaba#1815]define LifeCycle interface and related abstract class.

886fe6d

[alibaba#1815]add ResourceLifeCycleManager,adjust some code and fix s…

6981fbc

…ome unit test cases.

[alibaba#1815]add the related lifecycle test cases codes.

6d1baed

[alibaba#1815]add class file license.

2cb690a

[alibaba#1815]add class file license.

ab3b234

chuntaojun reviewed May 31, 2020

View reviewed changes

client/src/main/java/com/alibaba/nacos/client/config/http/ServerHttpAgent.java Outdated Show resolved Hide resolved

chuntaojun reviewed May 31, 2020

View reviewed changes

common/src/main/java/com/alibaba/nacos/common/lifecycle/ResourceLifeCycleManager.java Outdated Show resolved Hide resolved

wangweizZZ reviewed May 31, 2020

View reviewed changes

[alibaba#1815]adjust codes and reput the lifecycle to ConfigService c…

8d6dd8f

…lass level.

[alibaba#1815]adjust and optimize codes, reput the lifecycle to Confi…

2ab2742

…gService class level at the same time.

chuntaojun reviewed Jun 2, 2020

View reviewed changes

client/src/main/java/com/alibaba/nacos/client/config/http/ServerHttpAgent.java Outdated Show resolved Hide resolved

chuntaojun reviewed Jun 2, 2020

View reviewed changes

client/src/main/java/com/alibaba/nacos/client/config/impl/ClientWorker.java Outdated Show resolved Hide resolved

chuntaojun reviewed Jun 2, 2020

View reviewed changes

client/src/main/java/com/alibaba/nacos/client/config/impl/ClientWorker.java Show resolved Hide resolved

chuntaojun reviewed Jun 2, 2020

View reviewed changes

client/src/main/java/com/alibaba/nacos/client/config/impl/ServerListManager.java Outdated Show resolved Hide resolved

[alibaba#1815]adjust and optimize codes.

5e2840a

zongtanghu changed the title ~~Adding lifecycle interface and resolve the service downtime caused by thread leak~~ [ISSUE#1815]Adding lifecycle interface and resolve the service downtime caused by thread leak Jun 3, 2020

yanlinly requested a review from KomachiSion June 3, 2020 03:24

wangweizZZ approved these changes Jun 3, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into feature_lifecycle

0320e58

KomachiSion requested changes Jun 3, 2020

View reviewed changes

zongtanghu added 6 commits June 3, 2020 14:57

resolve codes conflicts

e86c158

Merge branch 'feature_lifecycle' of https://github.com/zongtanghu/nacos…

c726dd0

… into feature_lifecycle

[alibaba#1815]adjust this keywords which just includes constructor an…

09752e3

…d set method in the class and fix some unit test cases.

[alibaba#1815]fix unit test cases.

94c41e1

resolve codes conflicts

860a2d0

[alibaba#1815]remove destroyService from Factory Class.

15fad5d

KomachiSion reviewed Jun 3, 2020

View reviewed changes

api/src/main/java/com/alibaba/nacos/api/naming/NamingFactory.java Outdated Show resolved Hide resolved

KomachiSion approved these changes Jun 3, 2020

View reviewed changes

chuntaojun approved these changes Jun 4, 2020

View reviewed changes

[alibaba#1815]remove no used package name from Factory Class.

8a2921e

chuntaojun reviewed Jun 9, 2020

View reviewed changes

chuntaojun requested review from TsingLiang and yanlinly June 9, 2020 04:37

[alibaba#1815]remove no used package name from Factory Class and adju…

8f7fb0f

…st test cases.

yanlinly reviewed Jun 10, 2020

View reviewed changes

api/src/main/java/com/alibaba/nacos/api/exception/NacosException.java Outdated Show resolved Hide resolved

yanlinly reviewed Jun 10, 2020

View reviewed changes

client/src/main/java/com/alibaba/nacos/client/config/http/HttpAgent.java Outdated Show resolved Hide resolved

yanlinly reviewed Jun 10, 2020

View reviewed changes

common/src/main/java/com/alibaba/nacos/common/lifecycle/LifeCycle.java Outdated Show resolved Hide resolved

zongtanghu added 4 commits June 11, 2020 09:42

[alibaba#1815]remove lifeCycle, lifeCycleManager, and adjust some rel…

eb39830

…ated codes.

resolve the codes conflicts

e6e3161

[alibaba#1815]fix ut.

9b87379

[alibaba#1815]remove code comments.

d2b9806

yanlinly reviewed Jun 11, 2020

View reviewed changes

client/src/main/java/com/alibaba/nacos/client/config/impl/ServerListManager.java Outdated Show resolved Hide resolved

yanlinly reviewed Jun 11, 2020

View reviewed changes

client/src/main/java/com/alibaba/nacos/client/naming/backups/FailoverReactor.java Show resolved Hide resolved

[alibaba#1815]adjust codes for instance class attribute init.

e7796b3

yanlinly approved these changes Jun 11, 2020

View reviewed changes

yanlinly merged commit 4c38fe1 into alibaba:develop Jun 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ISSUE#1815]Adding lifecycle interface and resolve the service downtime caused by thread leak #2927

[ISSUE#1815]Adding lifecycle interface and resolve the service downtime caused by thread leak #2927

zongtanghu commented May 31, 2020 •

edited

Loading

zongtanghu commented May 31, 2020

wangweizZZ May 31, 2020

wangweizZZ May 31, 2020

wangweizZZ May 31, 2020

zongtanghu May 31, 2020

yfh0918 commented Jun 1, 2020

zongtanghu commented Jun 1, 2020

yfh0918 commented Jun 1, 2020

zongtanghu commented Jun 1, 2020

yfh0918 commented Jun 1, 2020

zongtanghu commented Jun 3, 2020

yfh0918 commented Jun 3, 2020

zongtanghu commented Jun 3, 2020

yfh0918 commented Jun 3, 2020

KomachiSion left a comment

zongtanghu commented Jun 3, 2020

KomachiSion left a comment

chuntaojun Jun 9, 2020

yanlinly Jun 10, 2020

zongtanghu Jun 11, 2020

yanlinly left a comment

[ISSUE#1815]Adding lifecycle interface and resolve the service downtime caused by thread leak #2927

[ISSUE#1815]Adding lifecycle interface and resolve the service downtime caused by thread leak #2927

Conversation

zongtanghu commented May 31, 2020 • edited Loading

What is the purpose of the change

Brief changelog

Verifying this change

zongtanghu commented May 31, 2020

wangweizZZ May 31, 2020

Choose a reason for hiding this comment

wangweizZZ May 31, 2020

Choose a reason for hiding this comment

wangweizZZ May 31, 2020

Choose a reason for hiding this comment

zongtanghu May 31, 2020

Choose a reason for hiding this comment

yfh0918 commented Jun 1, 2020

zongtanghu commented Jun 1, 2020

yfh0918 commented Jun 1, 2020

zongtanghu commented Jun 1, 2020

yfh0918 commented Jun 1, 2020

zongtanghu commented Jun 3, 2020

yfh0918 commented Jun 3, 2020

zongtanghu commented Jun 3, 2020

yfh0918 commented Jun 3, 2020

KomachiSion left a comment

Choose a reason for hiding this comment

zongtanghu commented Jun 3, 2020

KomachiSion left a comment

Choose a reason for hiding this comment

chuntaojun Jun 9, 2020

Choose a reason for hiding this comment

yanlinly Jun 10, 2020

Choose a reason for hiding this comment

zongtanghu Jun 11, 2020

Choose a reason for hiding this comment

yanlinly left a comment

Choose a reason for hiding this comment

zongtanghu commented May 31, 2020 •

edited

Loading