Add Timeout support #105
Add Timeout support #105
Conversation
Codecov Report
@@ Coverage Diff @@
## master #105 +/- ##
======================================
Coverage 100% 100%
======================================
Files 7 7
Lines 465 569 +104
======================================
+ Hits 465 569 +104
Continue to review full report at Codecov.
|
lib/etcdv3.rb
Outdated
| end | ||
|
|
||
| # Creates new user. | ||
| def user_add(user, password) | ||
| @conn.handle(:auth, 'user_add', [user, password]) | ||
| def user_add(user, password, opts={}) |
There was a problem hiding this comment.
Given these methods are directly accessed by the user, we are going to want these to be as clear as possible. Users, for the most part, should know exactly what can and cannot be passed into these methods by looking at this file.
With that being said, where a method doesn't allow multiple options we should just specify timeout instead of opts so it's clear that there's not additional options that can be specified. If opts makes the most sense like the case of the get method, then we need to document the available options.
I hope that make sense!
lib/etcdv3.rb
Outdated
| end | ||
|
|
||
| def role_revoke_permission(name, permission, key, range_end='') | ||
| @conn.handle(:auth, 'role_revoke_permission', [name, permission, key, range_end]) | ||
| def role_revoke_permission(name, permission, key, range_end='', opts={}) |
There was a problem hiding this comment.
@davissp14 do you have strong feelings about changing range_end here? It's kind of goofy the way it is now that an options hash is being passed in. It would break backwards compatibility, but it might be better if the method definition looked like
def role_revoke_permission(name, permission, key, range_end: range_end, timeout: timeout)
The other nice thing is it would make it consistent with get and del.
There was a problem hiding this comment.
You can certainly change that, not sure why I did that.
mgates
force-pushed
the
add_deadline_support_to_etcdv3
branch
2 times, most recently
from
311d773 to
181b28a
Compare
lib/etcdv3.rb
Outdated
| def put(key, value, lease_id: nil) | ||
| @conn.handle(:kv, 'put', [key, value, lease_id]) | ||
| # key - string | ||
| # optional :lease - integer |
|
Looks great guys. Let me know when you guys are wrapped up and i'll get this merged. |
|
On a side note, how do you guys feel about the global command timeout? |
|
Like, the concept of it or having it set to 120 seconds. I'm kinda lukewarm on the concept of it, but don't feel strongly. I have no opinions at all about 120 seconds as default. It seems kinda high, but I don't wanna surprise current users with something that's much shorter (we're probably going to set it to something like half a second, which probably isn't what most people want). |
mgates
force-pushed
the
add_deadline_support_to_etcdv3
branch
from
181b28a to
7044c1e
Compare
|
Do you guys intend on using the global timeout, or primarily going to be leveraging request based timeouts? |
|
I think probably the global one. |
|
Right on. Good to know. |
|
Aight, you ready for this to be merged? |
|
I think we're good. Thanks for all the help. |
|
Thank you! |
Ok, once more. This time we passed all of the options through from Etcdv3. We also turned some optional arguments into kwargs where it made sense (for internal methods only) and added a few extra tests.