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

consul lock #140

Merged
merged 19 commits into from
Nov 26, 2021
Merged

consul lock #140

merged 19 commits into from
Nov 26, 2021

Conversation

ZLBer
Copy link
Member

@ZLBer ZLBer commented Jul 17, 2021

What this PR does:
add consul lock
Which issue(s) this PR fixes:

Fixes #129

Special notes for your reviewer:
@seeflood
Does this PR introduce a user-facing change?:


@ZLBer
Copy link
Member Author

ZLBer commented Jul 17, 2021

@seeflood
cr没问题了我再添加完整的ut和doc吧

@codecov-commenter
Copy link

codecov-commenter commented Jul 17, 2021

Codecov Report

Merging #140 (f152baf) into main (5b5b8be) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #140   +/-   ##
=======================================
  Coverage   56.90%   56.90%           
=======================================
  Files          48       48           
  Lines        2100     2100           
=======================================
  Hits         1195     1195           
  Misses        774      774           
  Partials      131      131           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b5b8be...f152baf. Read the comment docs.

@seeflood
Copy link
Member

抱歉这周事(kai hui)太多,我明天看下!

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale label Oct 23, 2021
@github-actions
Copy link

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot closed this Oct 31, 2021
@seeflood
Copy link
Member

seeflood commented Nov 4, 2021

刚发现这个被机器人自动关了,@ZLBer 这个你还继续搞么~

@ZLBer
Copy link
Member Author

ZLBer commented Nov 5, 2021

@seeflood 时间有点久了..当时是有什么问题吗

@seeflood
Copy link
Member

seeflood commented Nov 5, 2021

@ZLBer 噢噢当时的遗留问题就是能否不用事务,因为用事务可能会有很多overhead
image

@ZLBer
Copy link
Member Author

ZLBer commented Nov 5, 2021

@seeflood 我这几天看下

@ZLBer ZLBer reopened this Nov 5, 2021
# Conflicts:
#	components/go.mod
#	components/go.sum
@ZLBer
Copy link
Member Author

ZLBer commented Nov 5, 2021

@seeflood 看了一下之前的方案确实麻烦了。unlock的时候可以直接删除session,这样和他绑定的lock就被释放掉了。但可能返回的状态不会很精确 ,比如没法区分 lock.LOCK_UNEXIST lock.LOCK_BELONG_TO_OTHER 这些。

@seeflood
Copy link
Member

seeflood commented Nov 5, 2021

但可能返回的状态不会很精确 ,比如没法区分 lock.LOCK_UNEXIST lock.LOCK_BELONG_TO_OTHER 这些。

@ZLBer 我查了下,用PUT xxxx?release=xxx 应该能知道是否已经被别人占用了~见
https://blog.didispace.com/spring-cloud-consul-lock-and-semphore/#lg=1&slide=0
https://www.consul.io/api/kv#create-update-key
image

@github-actions github-actions bot removed the stale label Nov 6, 2021
@ZLBer
Copy link
Member Author

ZLBer commented Nov 6, 2021

@seeflood release操作 LOCK_SUCCESS和LOCK_UNEXIST返回true,LOCK_BELONG_TO_OTHER返回false,无法区分前两种,倒是也没有太大影响。之前确实考虑太多了,那我就用最简单的acquire和release来重写?

@seeflood
Copy link
Member

seeflood commented Nov 6, 2021

@ZLBer 好呀

release操作 LOCK_SUCCESS和LOCK_UNEXIST返回true

恩这两个区分不了倒还好,就当LOCK_SUCCESS就行

@ZLBer ZLBer requested a review from seeflood November 6, 2021 08:34
components/lock/consul/consul_lock.go Outdated Show resolved Hide resolved
components/lock/consul/consul_lock_test.go Outdated Show resolved Hide resolved
components/lock/consul/consul_lock.go Outdated Show resolved Hide resolved
components/lock/consul/consul_lock.go Outdated Show resolved Hide resolved
components/lock/consul/consul_lock_test.go Show resolved Hide resolved
@zhenjunMa
Copy link
Contributor

@ZLBer
我看逻辑上面没什么问题了,merge前需要补充两点:

  1. 增加一个demo用法示例
  2. 单测中的地址链接要改成mock
  3. 分布式锁文档中应该需要增加一个consul组件的文档?

@ZLBer
Copy link
Member Author

ZLBer commented Nov 24, 2021

@zhenjunMa @seeflood Done

@zhenjunMa
Copy link
Contributor

@ZLBer Good job! thanks!

@zhenjunMa zhenjunMa merged commit faedd27 into mosn:main Nov 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Consul to implement Distributed Lock API
4 participants