Skip to content
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.

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

feature: add config support for seata server thread pool #3411

Merged
merged 6 commits into from
Jan 1, 2021

Conversation

lvxianzheng
Copy link
Contributor

Ⅰ. Describe what this PR did

新功能:增加seata server thread pool 可配置参数功能

Ⅱ. Does this pull request fix one issue?

增加参数配置,可以性能调优,tcc 模式下,当客户端并发超过50的时候,服务器和客户端都会Hang住,可以扩大此参数暂时解决问题,这是因为2阶段同步提交引起的,改为异步提交可以修复此问题。

Ⅲ. Why don't you add test cases (unit test/integration test)?

集成测试才可以测试

Ⅳ. Describe how to verify it

在file.conf中修改配置,运行性能测试,观察线程数量变化

Ⅴ. Special notes for reviews

@funky-eyes funky-eyes added first-time contributor first-time contributor module/server server module labels Dec 25, 2020
@funky-eyes
Copy link
Contributor

funky-eyes commented Dec 25, 2020

标题请用英文,可参考其他pr,请pull最新develop的代码并提交

@lvxianzheng lvxianzheng changed the title 新功能:增加seata server thread pool 可配置参数功能 feature: add config support for seata server thread pool Dec 25, 2020
@@ -1,4 +1,15 @@
## transaction log store, only used in seata-server
server {
Copy link
Contributor

Choose a reason for hiding this comment

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

I have some question about this pr.
1.The configuration ofNetty we already had the prefix name "transport.xxx.xxx", how about change to transport.xxx.xxx instead of server.xxx.xxx
2.How about move this configuration in the NettyServerConfig.java ?
3.We do not show the configuration of Netty in file.conf. How about remove it and add to

file.conf.example
script/server/file.conf
script/server/file.properties
script/server/file.yml

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

1 and 2:
I think the thread pool configurations are related to the server itself instead of transport.
So the server.xxx.xxx is more better than transport.
If the configuration is set for client and server communication, it is better using transport.xxx.xxx

  1. In fact, I can remove the default value in file.conf.
    I put the default value into the file.conf, because it is the responsibility to reminder we can change the value for performance problems.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for you replay. For now we already put the config of client in the NettyServerConfig and the server in the NettyServerConfig, and they begin with 'transport.xxx.xxx'. How do you think move the config of server to the NettyServerConfig and let it begin with 'transport.xxx.xxx'. May be you are right and I hope can keep in touch with after this. But for now I think the consistency is more important than the correctness.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now I put the configure into NettyServerConfig and change the name from server.xxx.xxx to transport.xxx.xxx

@codecov-io
Copy link

codecov-io commented Dec 31, 2020

Codecov Report

Merging #3411 (5de3153) into develop (8397e90) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #3411      +/-   ##
=============================================
- Coverage      51.67%   51.65%   -0.03%     
+ Complexity      3357     3354       -3     
=============================================
  Files            618      618              
  Lines          20344    20355      +11     
  Branches        2543     2544       +1     
=============================================
+ Hits           10513    10514       +1     
- Misses          8781     8790       +9     
- Partials        1050     1051       +1     
Impacted Files Coverage Δ Complexity Δ
...ava/io/seata/core/constants/ConfigurationKeys.java 0.00% <ø> (ø) 0.00 <0.00> (ø)
...ava/io/seata/core/rpc/netty/NettyServerConfig.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
server/src/main/java/io/seata/server/Server.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...o/seata/server/coordinator/DefaultCoordinator.java 54.63% <0.00%> (-0.52%) 28.00% <0.00%> (-1.00%)
...in/java/io/seata/server/session/GlobalSession.java 83.63% <0.00%> (-0.46%) 71.00% <0.00%> (-1.00%)
...n/src/main/java/io/seata/common/util/IdWorker.java 83.33% <0.00%> (+6.25%) 12.00% <0.00%> (+1.00%)

Copy link
Contributor

@l81893521 l81893521 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@l81893521 l81893521 left a comment

Choose a reason for hiding this comment

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

By the way. Can you add the change to the 1.5.0.md in the path /change and /change/en-us

Copy link
Member

@slievrly slievrly left a comment

Choose a reason for hiding this comment

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

LGTM

@slievrly slievrly merged commit 274fcd0 into apache:develop Jan 1, 2021
@slievrly slievrly added this to the 1.5.0 milestone Jan 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
first-time contributor first-time contributor module/server server module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants