-
Notifications
You must be signed in to change notification settings - Fork 8.9k
optimize: optimize seata client I/O processing by adjusting thread count #7170
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
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
|
https://github.com/apache/incubator-seata/blob/2.x/script/client/conf/file.conf#L44 |
| import static org.apache.seata.common.DefaultValues.DEFAULT_RPC_RM_REQUEST_TIMEOUT; | ||
| import static org.apache.seata.common.DefaultValues.DEFAULT_RPC_TM_REQUEST_TIMEOUT; | ||
| import static org.apache.seata.common.DefaultValues.DEFAULT_SELECTOR_THREAD_PREFIX; | ||
| import static org.apache.seata.common.DefaultValues.DEFAULT_SELECTOR_THREAD_SIZE; |
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.
The value of DEFAULT_SELECTOR_THREAD_SIZE should be changed to -1
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.
Done in f5d38a9 !
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.
Done in f5d38a9 !
In NettyClientBootstrap, it is necessary to handle the case where selectorThreadSizeThreadSize=-1.
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.
Done in f5d38a9 !
In NettyClientBootstrap, it is necessary to handle the case where selectorThreadSizeThreadSize=-1.
What do you think?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 2.x #7170 +/- ##
=========================================
Coverage 52.18% 52.18%
Complexity 6813 6813
=========================================
Files 1154 1154
Lines 41112 41113 +1
Branches 4818 4819 +1
=========================================
+ Hits 21455 21456 +1
+ Misses 17617 17616 -1
- Partials 2040 2041 +1
|
What do you think about changing the thread count to CPU cores * 2 when the value is negative? |
f5d38a9 to
e7cdebc
Compare
If it is a negative number, directly assign the value of |
| int threadSize = CONFIG.getInt(ConfigurationKeys.CLIENT_SELECTOR_THREAD_SIZE, WorkThreadMode.Default.getValue()); | ||
| return threadSize > 0 ? threadSize : WorkThreadMode.Default.getValue(); |
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.
Handling this in NettyClientBootstrap is a good idea, but I believe it is more appropriate to process it here.
Because if we handle this in NettyClientBootstrap, we would need to process -1 in every piece of code that retrieves the thread count using this method.
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.
You are right, handling it this way is better, good job.
|
I have resolved the failing test related to |
This is not caused by your PR; this issue is an occasional occurrence, and the exact cause is currently unknown. I have restarted the CI workflow for now. |
Ⅰ. Describe what this PR did
Ⅱ. Does this pull request fix one issue?
fixes #7168
Ⅲ. Why don't you add test cases (unit test/integration test)?
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews