-
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
optimize: @GlobalTransactional and @GlobalLock can now customize lock retry config #2962
Conversation
# Conflicts: # core/src/main/java/io/seata/core/context/GlobalLockConfigHolder.java # core/src/main/java/io/seata/core/model/GlobalLockConfig.java
Codecov Report
@@ Coverage Diff @@
## develop #2962 +/- ##
=============================================
+ Coverage 50.45% 50.54% +0.09%
- Complexity 3108 3122 +14
=============================================
Files 593 595 +2
Lines 19571 19637 +66
Branches 2427 2434 +7
=============================================
+ Hits 9874 9925 +51
- Misses 8702 8717 +15
Partials 995 995
|
int configInternal = config.getLockRetryInternal(); | ||
if (configInternal > 0) { | ||
retryInternal = configInternal; | ||
} |
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.
我觉得加上else retryInternal=ConfigurationFactory.getInstance().getInt(ConfigurationKeys.CLIENT_LOCK_RETRY_INTERVAL, DEFAULT_CLIENT_LOCK_RETRY_INTERVAL);
可以在有本地的锁重试时,无序再去加载配置中心中的配置
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.
有道理,我优化一下
int configTimes = config.getLockRetryTimes(); | ||
if (configTimes >= 0) { | ||
retryTimes = configTimes; | ||
} |
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.
同上,改动后LOCK_RETRY_TIMES和LOCK_RETRY_TIMES 似乎就可以删除
…stomized config is available
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
Please add the test. |
final GlobalLock globalLockAnno) throws Throwable { | ||
return globalLockTemplate.execute(new GlobalLockExecutor() { | ||
@Override | ||
public Object execute() throws Throwable { |
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.
Why remove the try 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.
Because the old try-catch in fact do nothing, just to wrap a throwable as a RuntimeException, so that it can match the outer method signature(throws Exception~). Here i simplify the method signature(throws Throwable), thus it can save the catch and wrap process. Just like its "brother" method(handleGlobalTransaction).
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.
ok
private static int LOCK_RETRY_INTERNAL = | ||
ConfigurationFactory.getInstance().getInt(ConfigurationKeys.CLIENT_LOCK_RETRY_INTERVAL, DEFAULT_CLIENT_LOCK_RETRY_INTERVAL); | ||
private static int LOCK_RETRY_TIMES = | ||
ConfigurationFactory.getInstance().getInt(ConfigurationKeys.CLIENT_LOCK_RETRY_TIMES, DEFAULT_CLIENT_LOCK_RETRY_TIMES); |
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.
Keep this constants. Returns it in the get methods, if there is no customized config.
} | ||
// if there is no customized config, use global config instead | ||
Configuration configuration = ConfigurationFactory.getInstance(); | ||
return configuration.getInt(ConfigurationKeys.CLIENT_LOCK_RETRY_INTERVAL, DEFAULT_CLIENT_LOCK_RETRY_INTERVAL); |
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.
Recovery LOCK_RETRY_INTERNAL
, and returns LOCK_RETRY_INTERNAL
.
} | ||
// if there is no customized config, use global config instead | ||
Configuration configuration = ConfigurationFactory.getInstance(); | ||
return configuration.getInt(ConfigurationKeys.CLIENT_LOCK_RETRY_TIMES, DEFAULT_CLIENT_LOCK_RETRY_TIMES); |
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.
Recovery LOCK_RETRY_TIMES
, and returns LOCK_RETRY_TIMES
.
…onfig(using ConfigurationChangeListener)
|
||
private int globalLockRetryInternal; | ||
|
||
private int globalLockRetryTimes; |
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.
volatile
} | ||
} | ||
|
||
private int parseInt(String value, int fallback) { |
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.
use the static method NumberUtils.toInt
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.
ok, i see it.
@@ -28,7 +32,7 @@ | |||
*/ | |||
public class LockRetryController { | |||
|
|||
private static final GlobalConfig globalConfig = new GlobalConfig(); | |||
private static GlobalConfig globalConfig = new GlobalConfig(); |
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.
常量的变量名用大写就好,不要把final拿掉。
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.
问题不大,反正别的地方也改不了它
# Conflicts: # spring/src/main/java/io/seata/spring/annotation/GlobalTransactionalInterceptor.java # tm/src/main/java/io/seata/tm/api/TransactionalTemplate.java
Codecov Report
@@ Coverage Diff @@
## develop #2962 +/- ##
=============================================
+ Coverage 50.40% 50.50% +0.09%
- Complexity 3123 3139 +16
=============================================
Files 594 596 +2
Lines 19698 19764 +66
Branches 2459 2466 +7
=============================================
+ Hits 9929 9982 +53
- Misses 8765 8780 +15
+ Partials 1004 1002 -2
|
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
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 for @wangliang181230
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.
随便问问, 啥也不影响
* note: 0 or negative number will take no effect(which mean fall back to global config) | ||
* @return | ||
*/ | ||
int lockRetryInternal() default 0; |
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.
lockRetryInternal 是不是应该叫做 lockRetryInterval 。 是拼错了, 还是本来就是这个意思?
Ⅰ. Describe what this PR did
allow @GlobalTransactional and @GlobalLock to customize lock retry interval and times, which would override the global config.
Ⅱ. Does this pull request fix one issue?
fixes #2943