-
Notifications
You must be signed in to change notification settings - Fork 8.9k
optimize: seata-server independent HTTP thread pool #7415
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
Conversation
|
I think we need to address the issue below. |
core/src/main/java/org/apache/seata/core/rpc/netty/http/HttpDispatchHandler.java
Outdated
Show resolved
Hide resolved
Strangely, I did not fail the pmd-check locally |
|
This branch has conflicts that must be resolved core/src/main/java/org/apache/seata/core/rpc/netty/http/HttpDispatchHandler.java |
# Conflicts: # core/src/main/java/org/apache/seata/core/rpc/netty/http/HttpDispatchHandler.java
funky-eyes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error: ClusterControllerTest.watch:97 expected: <200> but was: <500>
Error: ClusterControllerTest.watchTimeoutTest:66 expected: <304> but was: <500>
| Object[] args = ParameterParser.getArgValues(httpInvocation.getParamMetaData(), handleMethod, | ||
| requestDataNode, httpContext); | ||
| Object result = handleMethod.invoke(httpController, args); | ||
| Object result = handleMethod.invoke(httpController, args); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the code range that should be asynchronous is only between lines 132-138.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 2.x #7415 +/- ##
============================================
- Coverage 59.11% 59.09% -0.03%
Complexity 529 529
============================================
Files 1281 1281
Lines 46102 46142 +40
Branches 5562 5561 -1
============================================
+ Hits 27255 27267 +12
- Misses 16274 16312 +38
+ Partials 2573 2563 -10
🚀 New features to boost your workflow:
|
core/src/main/java/org/apache/seata/core/rpc/netty/http/HttpDispatchHandler.java
Outdated
Show resolved
Hide resolved
…spatchHandler.java
funky-eyes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| private static boolean ENABLE_TC_SERVER_BATCH_SEND_RESPONSE = CONFIG.getBoolean(ConfigurationKeys.ENABLE_TC_SERVER_BATCH_SEND_RESPONSE, | ||
| DefaultValues.DEFAULT_ENABLE_TC_SERVER_BATCH_SEND_RESPONSE); | ||
|
|
||
| private static int minHttpPoolSize = CONFIG.getInt(ConfigurationKeys.MIN_HTTP_POOL_SIZE, 10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/apache/incubator-seata/tree/2.x/seata-spring-autoconfigure/seata-spring-autoconfigure-server/src/main/java/org/apache/seata/spring/boot/autoconfigure/properties/server
Need to supplement the properties class for obtaining configurations, and also add configuration items to these three configuration file
https://github.com/apache/incubator-seata/blob/2.x/server/src/main/resources/application.example.yml
https://github.com/apache/incubator-seata/blob/2.x/server/src/main/resources/application.raft.example.yml
https://github.com/apache/incubator-seata/blob/2.x/script/config-center/config.txt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@funky-eyes I have a little bit of a question
Currently, the thread pool for handling Seata and gRPC protocols uses System.getProperty for configuration:
private static int minServerPoolSize = Integer.parseInt(System.getProperty(ConfigurationKeys.MIN_SERVER_POOL_SIZE, "50"));
private static int maxServerPoolSize = Integer.parseInt(System.getProperty(ConfigurationKeys.MAX_SERVER_POOL_SIZE, "500"));
private static int maxTaskQueueSize = Integer.parseInt(System.getProperty(ConfigurationKeys.MAX_TASK_QUEUE_SIZE, "20000"));
private static int keepAliveTime = Integer.parseInt(System.getProperty(ConfigurationKeys.KEEP_ALIVE_TIME, "500"));
In older versions, System.getProperty was used for configurations, which seemingly limited modifications to only through JVM startup parameters in the command script.
Conversely, the HTTP thread pool is configured using CONFIG.getInt:
private static int minHttpPoolSize = CONFIG.getInt(ConfigurationKeys.MIN_HTTP_POOL_SIZE, 10);
private static int maxHttpPoolSize = CONFIG.getInt(ConfigurationKeys.MAX_HTTP_POOL_SIZE, 100);
private static int maxHttpTaskQueueSize = CONFIG.getInt(ConfigurationKeys.MAX_HTTP_TASK_QUEUE_SIZE, 1000); private static int httpKeepAliveTime = CONFIG.getInt(ConfigurationKeys.HTTP_POOL_KEEP_ALIVE_TIME, 500);
CONFIG.getInt supports various configuration sources, including system properties, environment variables, configuration files (e.g., Spring Boot's application.yaml), and configuration centers.
To fully leverage this, these configurations should be integrated into the seata.spring.boot.autoconfigure module(for configuration files) and added to seata/blob/2.x/script/config-center/config.txt(for config-center).
Therefore, I believe if the HTTP message handling thread pool configuration uses CONFIG.getInt, then the thread pools for Seata and gRPC messages should also switch to this configuration method. This would enable support for Spring Boot's application.yaml and integrated configuration center modes.
Additionally, I think all System.getProperty usages should be updated to CONFIG.getInt, as CONFIG.getInt encompasses all functionalities of System.getProperty.
Can you answer my confusion, thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with your point. You can submit a new PR to make the thread pool configurations for Seata and gRPC read from CONFIG as well.
Ⅰ. Describe what this PR did
Currently, seata-server uses netty to manage network requests, and HTTP request processing is executed synchronously with the io thread pool of netty itself. At the same time, due to the mechanism of multiple protocols on a single port, blocking may occur under high concurrency.
Therefore, an independent thread pool is added to process HTTP requests for processing of HTTP requests asynchronously. (align seata, grpc protocol asynchronous processing)
Ⅱ. Does this pull request fix one issue?
#7405
Ⅲ. Why don't you add test cases (unit test/integration test)?
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews