-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
bugfix: fix AsyncWorker potential OOM problem #3258
bugfix: fix AsyncWorker potential OOM problem #3258
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #3258 +/- ##
=============================================
+ Coverage 51.44% 51.56% +0.11%
- Complexity 3327 3329 +2
=============================================
Files 616 616
Lines 20206 20192 -14
Branches 2536 2529 -7
=============================================
+ Hits 10395 10412 +17
+ Misses 8766 8736 -30
+ Partials 1045 1044 -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.
LGTM, 给我的话,做法是达到指定循环次数就跳出循环。 目前你的做法更棒,甘拜下风。
修改的内容如下:
配置中心的缓冲区配置,虽然不知道有没有用, 配上总是没错的。
用drainTo的方式+toMap提前确认数据源需要操作的数据。
Lists.partition 的方式逻辑分割数据。
rm-datasource/src/main/java/io/seata/rm/datasource/AsyncWorker.java
Outdated
Show resolved
Hide resolved
rm-datasource/src/main/java/io/seata/rm/datasource/AsyncWorker.java
Outdated
Show resolved
Hide resolved
rm-datasource/src/main/java/io/seata/rm/datasource/AsyncWorker.java
Outdated
Show resolved
Hide resolved
rm-datasource/src/main/java/io/seata/rm/datasource/AsyncWorker.java
Outdated
Show resolved
Hide resolved
DataSourceManager resourceManager = (DataSourceManager) DefaultResourceManager.get().getResourceManager(BranchType.AT); | ||
DataSourceProxy dataSourceProxy = resourceManager.get(resourceId); | ||
if (dataSourceProxy == null) { | ||
LOGGER.warn("Failed to find resource for {}", resourceId); |
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.
这里是不是应该直接抛出异常?而不是打了日志之后return?
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.
这里应该不能抛出异常, 抛出异常的话for循环处理就中断了。
在多数据源的场景下,第一个数据源找不到影响第二个数据源无法提交也不合理。
是不是搞个大catch,让这个dealWithGroupedContexts不抛出异常比较好?
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.
大而全地catch的话,不好区分异常发生的地方(以便针对性地输出日志)
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.
分组函数有点丑
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.
rm-datasource/src/main/java/io/seata/rm/datasource/AsyncWorker.java
Outdated
Show resolved
Hide resolved
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
rm-datasource/src/main/java/io/seata/rm/datasource/AsyncWorker.java
Outdated
Show resolved
Hide resolved
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
Ⅰ. Describe what this PR did
Fix AsyncWorker may cause OOM when TC continuously send async commit message to RM.
Ⅱ. Does this pull request fix one issue?
fixes #3240
Ⅳ. Describe how to verify it
use method descripted in the issue.