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 support for native ETCD distributed locks #122

Merged
merged 15 commits into from
Dec 18, 2018

Conversation

dcepelik
Copy link
Contributor

Hi Shaun,

as promised.

One new test case is commented out as it's failing, this is because you have a sophisticated helper in place which I wanted to leverage but which cannot be used for the lock gRPC call (obviously it will time out, because it's locked from the previous call in the same helper call). I don't know the proper Ruby way of making this work (unlocking in the helper), so I was hoping you would have the capacity to fix that one test case, or at least provide some hints how to do it.

I've done extensive local testing and everything seems to work well. The semantics of the locking/unlocking are really simple, as the RPC call will fail, or a time-out will occur, or the locking/unlocking has succeeded. Still this deserves a thorough examination, because I'm not much of a Ruby programmer and have been fighting with the tools. (For example, I did not find a way to convince gRPC compiler to generate relative requires, so I had to fix them manually.)

One more note, most pieces of protobuf code were taken from the official ETCD github repo, but it's not mentioned anywhere. (But I suspect that's the case for other protobuf code in the gem as well.)

Please let me know if I can do more.

David

@codecov
Copy link

codecov bot commented Nov 22, 2018

Codecov Report

Merging #122 into master will increase coverage by 0.09%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #122      +/-   ##
==========================================
+ Coverage   98.35%   98.45%   +0.09%     
==========================================
  Files          24       29       +5     
  Lines        1521     1614      +93     
==========================================
+ Hits         1496     1589      +93     
  Misses         25       25
Impacted Files Coverage Δ
lib/etcdv3/connection.rb 91.3% <ø> (ø) ⬆️
lib/etcdv3/auth.rb 100% <100%> (ø) ⬆️
lib/etcdv3/lock.rb 100% <100%> (ø)
spec/etcdv3_spec.rb 100% <100%> (ø) ⬆️
lib/etcdv3/etcdrpc/v3lock_services_pb.rb 100% <100%> (ø)
lib/etcdv3/etcdrpc/annotations_pb.rb 100% <100%> (ø)
lib/etcdv3/kv.rb 96.87% <100%> (+0.1%) ⬆️
lib/etcdv3.rb 98.97% <100%> (+0.12%) ⬆️
lib/etcdv3/etcdrpc/rpc_pb.rb 100% <100%> (ø) ⬆️
lib/etcdv3/etcdrpc/v3lock_pb.rb 100% <100%> (ø)
... and 10 more

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 c5e754d...854705b. Read the comment docs.

Tomas Volf and others added 9 commits December 10, 2018 17:06
Grpc was being kept at 1.6.0, which does not support ruby 2.5.0. Since
that important for us, update to 1.17.0 and solve issue with deadline
in spec by using `#from_relative_time` provided by
`GRPC::Core::TimeConsts` instead of our own implementation.

Also provide rake file with task to download etcd and improve travis
config to test more configurations.
@graywolf-at-work
Copy link
Contributor

If this is merged, #115 can be closed since this fixes the same issue.

Copy link
Owner

@davissp14 davissp14 left a comment

Choose a reason for hiding this comment

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

Small changes need with the readme, but otherwise looks solid.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
lib/etcdv3/auth.rb Show resolved Hide resolved
@dcepelik
Copy link
Contributor Author

Fixed.

@davissp14 davissp14 merged commit 6bb309f into davissp14:master Dec 18, 2018
@davissp14
Copy link
Owner

Nice work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants