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

add mongo distributed lock #348

Merged
merged 21 commits into from
Dec 15, 2021
Merged

add mongo distributed lock #348

merged 21 commits into from
Dec 15, 2021

Conversation

LXPWing
Copy link
Member

@LXPWing LXPWing commented Dec 3, 2021

What this PR does:
add mongo distributed lock

Which issue(s) this PR fixes:
#104

Copy link
Member

@seeflood seeflood left a comment

Choose a reason for hiding this comment

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

demo/configuration/apollo/apolloClientDemo
There is no need to submit this binary file :)

@seeflood
Copy link
Member

seeflood commented Dec 3, 2021

Thanks for your contribution!

@seeflood
Copy link
Member

seeflood commented Dec 3, 2021

关于如何在ut中mock掉mongoDB:
我看mongo的sdk是返回一个叫 mongo.Client的struct对吧。struct不好mock,所以一般写ut的关键是:

  • 先自己把struct封装成一个interface。具体来说,先定义一个interface,然后你的NewMongoClient(m MongoMetadata)这个函数应该返回这个interface,而不是mongo.Client这个struct
  • 一但封装成interface了,在ut里就可以把他替换成自己的mock实现了

可以参考 @ZLBer 之前写的zookeeper lock

他当时也面临同样的问题(如何在ut里mock zk),解决方案是把zk 的connection封装成一个interface:

type ZKConnection interface {

@seeflood
Copy link
Member

seeflood commented Dec 3, 2021

@ZLBer @whalesongAndLittleFish @stulzq Hi guys, could u help review this PR?

components/lock/mongo/mongo_lock.go Outdated Show resolved Hide resolved
components/lock/mongo/mongo_lock.go Show resolved Hide resolved
components/lock/mongo/mongo_lock.go Outdated Show resolved Hide resolved
components/lock/mongo/mongo_lock.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Dec 7, 2021

Codecov Report

Merging #348 (07a455c) into main (eb92185) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #348   +/-   ##
=======================================
  Coverage   57.62%   57.62%           
=======================================
  Files          48       48           
  Lines        2289     2289           
=======================================
  Hits         1319     1319           
  Misses        837      837           
  Partials      133      133           

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 eb92185...07a455c. Read the comment docs.

@LXPWing LXPWing requested a review from ZLBer December 7, 2021 15:27
@ZLBer
Copy link
Member

ZLBer commented Dec 9, 2021

@LXPWing no big problem, next ,you need to add docs in docs/_sidebar.md docs/en/component_specs/lock/XX.md docs/zh/component_specs/lock/XX.md.by the way ,the mock can use gomock, it‘s quick.
thanks for your contribution.

Copy link
Contributor

@whalesongAndLittleFish whalesongAndLittleFish left a comment

Choose a reason for hiding this comment

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

nice~,LGTM

@seeflood
Copy link
Member

seeflood commented Dec 14, 2021

Sorry for not replying !
I found that there are many ut failures in components package (including some compilation errors)
Our github workflow doesn't test the components package ,so we didn't know the issue until I recently read the code.....

I will submit a PR to fix the test cases and then go back to test and merge this PR.
Thanks!

@seeflood seeflood merged commit e86ff7f into mosn:main Dec 15, 2021
@seeflood
Copy link
Member

Thanks again for your contribution!
This component will be included in layotto v0.4

@ZLBer
Copy link
Member

ZLBer commented Dec 15, 2021

@LXPWing Thanks for your contribution! Maybe you can try another community task~

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.

5 participants