-
Notifications
You must be signed in to change notification settings - Fork 230
Dev/add etcd implementation #247
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
base: master
Are you sure you want to change the base?
Dev/add etcd implementation #247
Conversation
Really appreciate the great work @thangnguyen-69 ! I've gone through the code and detail and added a number of comments. I think there are a bunch of small things to do but overall the code looks pretty close! 2 other asks: |
var lock2 = new EtcdLeaseDistributedLock(client, "etcd"); | ||
await using var handle2 = await lock2.TryAcquireAsync(); | ||
Assert.That(handle2, Is.Not.Null, "Failed to acquire lock"); | ||
} |
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.
I don't think these 3 test cases add anything that isn't covered by the combinatorial cases. Instead, use this class to test for any Etcd-specific behavior.
Notably, we should test all constructor argument validations here, including for the options classes!
src/DistributedLock.Tests/Tests/Etcd/EtcdSynchronizationProviderTest.cs
Outdated
Show resolved
Hide resolved
src/DistributedLock.Tests/Tests/Etcd/EtcdSynchronizationProviderTest.cs
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.
Thanks for the contribution @thangnguyen-69 ! This is off to a great start!
f37dbd6
to
854cb97
Compare
@madelson received your comments and i will work on it and notify back again for a 2nd round. |
For #119