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

bugfix: reduce unnecessary competition and remove missing locks #3448

Merged
merged 9 commits into from
Feb 2, 2021

Conversation

funky-eyes
Copy link
Contributor

Ⅰ. Describe what this PR did

1.当我抢到了1个锁时,失败了2个,而succes[0]这下标只会记录一个key,导致会有锁被遗漏删除
2.仅需竞争key rowkey,如果失败时del会一并删除,所以没必要让redis处理其他field的竞争上
3.因为指令是有序响应,仅需get(0)拿到setnx的信息比较即可

Ⅱ. Does this pull request fix one issue?

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

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@codecov-io
Copy link

codecov-io commented Jan 7, 2021

Codecov Report

Merging #3448 (dd46281) into develop (23751d6) will decrease coverage by 0.01%.
The diff coverage is 70.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #3448      +/-   ##
=============================================
- Coverage      51.57%   51.55%   -0.02%     
+ Complexity      3379     3373       -6     
=============================================
  Files            619      619              
  Lines          20468    20467       -1     
  Branches        2563     2564       +1     
=============================================
- Hits           10556    10552       -4     
  Misses          8852     8852              
- Partials        1060     1063       +3     
Impacted Files Coverage Δ Complexity Δ
...eata/core/store/db/AbstractDataSourceProvider.java 0.00% <ø> (ø) 0.00 <0.00> (ø)
...seata/server/storage/redis/JedisPooledFactory.java 21.27% <ø> (ø) 4.00 <0.00> (ø)
...o/seata/server/storage/redis/lock/RedisLocker.java 63.06% <70.00%> (-0.33%) 18.00 <0.00> (ø)
...n/src/main/java/io/seata/common/util/IdWorker.java 77.08% <0.00%> (-6.25%) 11.00% <0.00%> (-1.00%)
...torage/file/store/FileTransactionStoreManager.java 56.77% <0.00%> (-0.65%) 28.00% <0.00%> (-1.00%)
.../java/io/seata/server/coordinator/DefaultCore.java 48.19% <0.00%> (-0.61%) 24.00% <0.00%> (-1.00%)
...rage/redis/store/RedisTransactionStoreManager.java 63.92% <0.00%> (-0.46%) 37.00% <0.00%> (-1.00%)
...erver/storage/file/session/FileSessionManager.java 51.82% <0.00%> (+2.91%) 26.00% <0.00%> (+1.00%)

@slievrly slievrly self-requested a review January 10, 2021 03:30
@funky-eyes funky-eyes added this to the 1.5.0 milestone Jan 11, 2021
@funky-eyes funky-eyes added module/server server module theme: storage Relate to seata store type: bug Category issues or prs related to bug. TC/store store mode and removed theme: storage Relate to seata store labels Jan 11, 2021
if (success.length > 0) {
jedis.del(success);
if (success.size() > 0) {
jedis.del(success.toArray(new String[0]));
Copy link
Contributor

Choose a reason for hiding this comment

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

是否可以使用lua

Copy link
Contributor Author

Choose a reason for hiding this comment

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

纯del 无需lua

Copy link
Contributor

Choose a reason for hiding this comment

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

纯del 无需lua
我是指这里整个加锁动作,封装到一个lua里,这样就不会存在del操作了

Copy link
Contributor Author

Choose a reason for hiding this comment

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

纯del 无需lua
我是指这里整个加锁动作,封装到一个lua里,这样就不会存在del操作了

这个得后续优化了,把操作写成纯lua来原子执行

for (int i = 0; i < partitions.size(); i++) {
String key = readyKeys.get(i);
if (partitions.get(i).contains(FAILED)) {
if (Objects.equals(partitions.get(i).get(0),FAILED)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

partitions.get(i) 里面放了七个hset的结果。contains可以发现任何一个错误,如果是partitions.get(i).get(0),那等于是只比较pipeline.hsetnx(key, XID, value.getXid())操作的返回结果了。有没有可能,hset xid这个操作成功了,但是后面的hset失败了。那这样比较就出问题了。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

有hsetnx可能失败,hset没理由失败,除非redis突然宕机了,如果算突然宕机即使都是hsetnx也可能出现这个问题

Copy link
Contributor

@lightClouds917 lightClouds917 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

@ph3636 ph3636 left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -119,29 +119,29 @@ public boolean acquireLock(List<RowLock> rowLocks) {
pipeline.hsetnx(key, XID, value.getXid());
pipeline.hsetnx(key, TRANSACTION_ID, value.getTransactionId().toString());
pipeline.hsetnx(key, BRANCH_ID, value.getBranchId().toString());
Copy link
Contributor

Choose a reason for hiding this comment

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

Hsetnx doesn't work. Why don't we replace it with hset

Copy link
Contributor Author

Choose a reason for hiding this comment

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

如果锁已经被其他的事务持有,那么如果用hset会导致后续解锁时失败,会串改锁持有者的信息,而下面的4个信息无论持有者是谁都没有影响,是固定的 @ph3636

Copy link
Member

@xingfudeshi xingfudeshi 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.

LGTM

@l81893521 l81893521 merged commit 863e68a into apache:develop Feb 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module/server server module TC/store store mode type: bug Category issues or prs related to bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants