Skip to content

feat: support triple thread monitor #711

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
glmapper merged 9 commits into from
Oct 10, 2020
Merged

feat: support triple thread monitor #711

glmapper merged 9 commits into from
Oct 10, 2020

Conversation

@OrezzerO
Copy link
Contributor

@OrezzerO OrezzerO commented Sep 9, 2020 •
edited by ujjboy
Loading

feat: support triple thread monitor.

@codecov-commenter
Copy link

codecov-commenter commented Sep 30, 2020 •
edited
Loading

Codecov Report

Merging #711 into master will increase coverage by 18.82%.
The diff coverage is 81.25%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master     #711       +/-   ##
=============================================
+ Coverage     51.74%   70.56%   +18.82%     
- Complexity       78       84        +6     
=============================================
  Files           291      296        +5     
  Lines          8187     8317      +130     
  Branches       1142     1157       +15     
=============================================
+ Hits           4236     5869     +1633     
+ Misses         3500     1759     -1741     
- Partials        451      689      +238
Impacted Files Coverage Δ Complexity Δ
...a/com/alipay/sofa/rpc/boot/log/LoggerConstant.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...pay/sofa/rpc/boot/common/RpcThreadPoolMonitor.java 70.73% <77.77%> (+70.73%) 0.00 <0.00> (ø)
...sofa/rpc/boot/container/ServerConfigContainer.java 79.19% <100.00%> (+69.15%) 0.00 <0.00> (ø)
.../sofa/isle/spring/config/SofaModuleProperties.java 79.16% <0.00%> (-10.84%) 0.00% <0.00%> (ø%)
.../configure/SofaRuntimeConfigurationProperties.java 94.11% <0.00%> (-5.89%) 0.00% <0.00%> (ø%)
...sofa/runtime/ext/component/ExtensionComponent.java 59.25% <0.00%> (-2.28%) 0.00% <0.00%> (ø%)
...tation/PlaceHolderAnnotationInvocationHandler.java 77.77% <0.00%> (-2.23%) 0.00% <0.00%> (ø%)
...m/alipay/sofa/isle/stage/ModuleLogOutputStage.java 86.66% <0.00%> (-1.91%) 0.00% <0.00%> (ø%)
...ofa/runtime/spring/factory/ServiceFactoryBean.java 72.97% <0.00%> (-0.72%) 0.00% <0.00%> (ø%)
...utoconfigure/isle/SofaModuleAutoConfiguration.java 88.88% <0.00%> (ø) 0.00% <0.00%> (ø%)
... and 102 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7cbdc2d...b0a7df0. Read the comment docs.

ujjboy
ujjboy requested changes Oct 1, 2020
View reviewed changes
...re/rpc-sofa-boot/src/main/java/com/alipay/sofa/rpc/boot/container/ServerConfigContainer.java Outdated

if (threadPoolExecutor != null) {
new RpcThreadPoolMonitor(threadPoolExecutor).start();
new RpcThreadPoolMonitor(threadPoolExecutor, LoggerConstant.BOLT_THREAD_LOGGER_NAME)
Copy link
Member

@ujjboy ujjboy Oct 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个对象要记录下来，start 的时候 new 出来的话， stop 的时候要销毁

Copy link
Contributor Author

@OrezzerO OrezzerO Oct 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已修复 @ujjboy

...re/rpc-sofa-boot/src/main/java/com/alipay/sofa/rpc/boot/container/ServerConfigContainer.java Outdated
ThreadPoolExecutor threadPoolExecutor = tripleServer.getBizThreadPool();

// 加入线程监测？
new RpcThreadPoolMonitor(threadPoolExecutor, LoggerConstant.TRIPLE_THREAD_LOGGER_NAME)
Copy link
Member

@ujjboy ujjboy Oct 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个对象要记录下来，start 的时候 new 出来的话， stop 的时候要销毁

@OrezzerO OrezzerO requested a review from ujjboy October 9, 2020 03:13
@codecov-io
Copy link

codecov-io commented Oct 9, 2020 •
edited
Loading

Codecov Report

Merging #711 into master will increase coverage by 18.94%.
The diff coverage is 82.14%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master     #711       +/-   ##
=============================================
+ Coverage     51.74%   70.68%   +18.94%     
- Complexity       78       84        +6     
=============================================
  Files           291      296        +5     
  Lines          8187     8350      +163     
  Branches       1142     1163       +21     
=============================================
+ Hits           4236     5902     +1666     
+ Misses         3500     1758     -1742     
- Partials        451      690      +239
Impacted Files Coverage Δ Complexity Δ
...a/com/alipay/sofa/rpc/boot/log/LoggerConstant.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...sofa/rpc/boot/container/ServerConfigContainer.java 78.09% <79.16%> (+68.05%) 0.00 <0.00> (ø)
...pay/sofa/rpc/boot/common/RpcThreadPoolMonitor.java 86.15% <84.74%> (+86.15%) 0.00 <0.00> (ø)
.../sofa/isle/spring/config/SofaModuleProperties.java 79.16% <0.00%> (-10.84%) 0.00% <0.00%> (ø%)
.../configure/SofaRuntimeConfigurationProperties.java 94.11% <0.00%> (-5.89%) 0.00% <0.00%> (ø%)
...sofa/runtime/ext/component/ExtensionComponent.java 59.25% <0.00%> (-2.28%) 0.00% <0.00%> (ø%)
...tation/PlaceHolderAnnotationInvocationHandler.java 77.77% <0.00%> (-2.23%) 0.00% <0.00%> (ø%)
...ofa/runtime/spring/factory/ServiceFactoryBean.java 72.97% <0.00%> (-0.72%) 0.00% <0.00%> (ø%)
...utoconfigure/isle/SofaModuleAutoConfiguration.java 88.88% <0.00%> (ø) 0.00% <0.00%> (ø%)
...utoconfigure/startup/SofaStartupConfiguration.java 46.15% <0.00%> (ø) 0.00% <0.00%> (ø%)
... and 100 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7cbdc2d...9e439a6. Read the comment docs.

ujjboy
ujjboy requested changes Oct 9, 2020
View reviewed changes
...t-core/rpc-sofa-boot/src/main/java/com/alipay/sofa/rpc/boot/common/RpcThreadPoolMonitor.java Outdated
sleep(30000);
} catch (InterruptedException e) {
LOGGER.error("Error happen the thread pool watch sleep ");
logger.error("Error happen the thread pool watch sleep ");
Copy link
Member

@ujjboy ujjboy Oct 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这句错误的英文改下吧，读起来不通顺啊。

Copy link
Contributor Author

@OrezzerO OrezzerO Oct 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已修改

...t-core/rpc-sofa-boot/src/main/java/com/alipay/sofa/rpc/boot/common/RpcThreadPoolMonitor.java Outdated
}

try {
sleep(30000);
Copy link
Member

@ujjboy ujjboy Oct 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个值 30000 建议做成一个属性 或者一个配置项

Copy link
Contributor Author

@OrezzerO OrezzerO Oct 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已修改

ujjboy
ujjboy requested changes Oct 9, 2020
View reviewed changes
...t-core/rpc-sofa-boot/src/main/java/com/alipay/sofa/rpc/boot/common/RpcThreadPoolMonitor.java Outdated
if (this.monitor != null) {
this.monitor.interrupt();
}
this.threadPoolExecutor = null;
Copy link
Member

@ujjboy ujjboy Oct 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

start() stop() 能随意调的话，这个 threadPoolExecutor 可能不需要置为 null。

Copy link
Contributor Author

@OrezzerO OrezzerO Oct 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我添加了 threadPoolExecutor 的set方法，每次调用前重新设置 threadPoolExecutor 吧

...t-core/rpc-sofa-boot/src/main/java/com/alipay/sofa/rpc/boot/common/RpcThreadPoolMonitor.java Outdated
}

public void stop() {
this.active = false;
Copy link
Member

@ujjboy ujjboy Oct 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

想了下 start() stop() 如果被并发调的话， active 换成 AtomicBoolean 是不是更合理一些？

Copy link
Contributor Author

@OrezzerO OrezzerO Oct 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

改成AtomicBoolean 也不能保证并发情况下的线程安全，我在 start 和 stop 方法上加了两个 synchronized 锁来保证线程安全

ujjboy
ujjboy approved these changes Oct 10, 2020
View reviewed changes
Copy link
Member

@ujjboy ujjboy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

glmapper
glmapper approved these changes Oct 10, 2020
View reviewed changes
Copy link
Contributor

@glmapper glmapper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@glmapper glmapper merged commit 6942f87 into sofastack:master Oct 10, 2020
@alaneuler alaneuler added this to the 3.4.5 milestone Oct 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ujjboy ujjboy ujjboy approved these changes

@glmapper glmapper glmapper approved these changes

@alaneuler alaneuler Awaiting requested review from alaneuler

@HzjNeverStop HzjNeverStop Awaiting requested review from HzjNeverStop

Assignees

No one assigned

Labels

cla:yes size/L

Projects

None yet

Milestone

3.4.5

Development

Successfully merging this pull request may close these issues.

6 participants

@OrezzerO @codecov-commenter @codecov-io @ujjboy @glmapper @alaneuler