Add Timeout support

[mgates]

This adds support for GRPC timeouts to KV operations. If this looks good, we'll add it to the other operations and add docs for it, but we wanted to run it up the flagpole first.

[codecov[bot]]

Codecov Report

Merging #99 into master will not change coverage. The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #99   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           7      7           
  Lines         465    506   +41     
=====================================
+ Hits          465    506   +41

Impacted Files	Coverage Δ
spec/etcdv3_spec.rb	100% <100%> (ø)	:arrow_up:
spec/etcdv3/lease_spec.rb	100% <100%> (ø)	:arrow_up:
spec/etcdv3/auth_spec.rb	100% <100%> (ø)	:arrow_up:
spec/etcdv3/kv_spec.rb	100% <100%> (ø)	:arrow_up:
spec/etcdv3/connection_wrapper_spec.rb	100% <100%> (ø)	:arrow_up:
spec/etcdv3/connection_spec.rb	100% <100%> (ø)	:arrow_up:

---

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 ede26ae...6addccd. Read the comment docs.

[mgates]

So, the docs are here, but I haven't found the actual implementation, much less figure out if it's possible to pass the different kinds of timeouts in. I'm inclined to let the backoff algorithm do it's thing, and to just have a had cap (like this) for cases where that much latency is unacceptable.

I'll add a default timeout (30 seconds? 120?) to clean this up and add it to the test of the request types.

Thanks for the thoughts and for being so responsive.

[davissp14]

Yeah, that sounds reasonable.

Do we want to follow suit with etcdctl and set the default command timeout to 5 seconds?

[mgates]

Sure? That might be something worth bumping a version though, casue it might break people's existing setups.

[davissp14]

Agreed. Whatever timeout you think is best, is good with me. I'm planning on cutting a new release when #90 gets merged. I figured i'd let you get this stuff in first so you don't have to deal with the merge conflicts. :)

[mgates]

OK, I added it for the rest of the requests, I'm a bit unhappy with all the boiler plate, but it's probably better than doing something tricky to wrap all the stubs. I also didn't add it to watches, cause I wasn't super sure how it would interact, but I can do some testing at some point.

[davissp14]

I think it might make sense to take a step back and look closer at how users would want to take advantage of the command timeout.

1. I think we may want to be clear that this is a command based timeout and not a connection timeout. timeout may be a little confusing given we are setting this on the main initializer where dial timeouts are usually issued.

2. I think setting a global command timeout is fine, but we will also want to support the ability to override the timeout per request. We don't want users having to re-initialize their connection each time they want to set a new timeout for a given command.

Usage examples:

# Sets a global default command timeout for all commands issued under this connection.  
conn = Etcdv3.new(url: 'http://localhost:2379', command_timeout: 5) 

# Assigns a timeout on a given operation, which would override the default timeout assigned on the main initializer.  
# This would be particularly useful for maintenance related commands like `defrag`, where it's expected to take longer than the default command timeout.
conn.get('a', range_end: 'ZZZZZ', timeout: 10)

thoughts?

[mgates]

Yeah - I think I for sure leaned too hard into not changing the api at all - I think that that's not a huge deal as long as people don't want to pass arbitrary metadata in, which, as far as I can tell, there would be no reason to. I think 'command_timeout' is a good name.

[davissp14]

Just a heads up, i'll be merging multiple endpoint support here in a few. If you need any help dealing with merge conflicts, let me know.

[mgates]

I'll work it out, I don't think it'll be a problem - sorry for the slowness, we've had other stuff that needs attention, we should be back to it next week.

[davissp14]

Right on and no worries at all!

[mgates]

Hey @davissp14 - We finally got back to it. If this looks good, should we bump the version here or in a separately?

[davissp14]

In reference to the version bump, i'll cut a new version once this stuff gets merged.

[mgates]

Oops, lots of boiler-plate :). We'll fix it up.

[mgates]

Ok - updated LMK if you want it squashed.

[davissp14]

SO I don't think request based timeouts actually works here. I merged and will fix it soon.

[mgates]

Ok - let us know, and we'll figure out what will make it work this week.

[davissp14]

https://github.com/davissp14/etcdv3-ruby/blob/master/lib/etcdv3.rb#L80-L92

There's nothing that passes the timeout argument here: https://github.com/davissp14/etcdv3-ruby/blob/master/lib/etcdv3/kv.rb#L11-L21

[mgates]

That's...odd. It seems to work, I'll puzzle out what's going on.

[mgates]

Oh geez, I'm a dummy - that only works with get because it passes the options hash. Will fix.

[davissp14]

Haha, no worries.

On Tue, Oct 3, 2017 at 9:05 AM Micah Gates notifications@github.com wrote:

Oh geez, I'm a dummy - that only works with get because it passes the options hash. Will fix.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/davissp14/etcdv3-ruby/pull/99#issuecomment-333852576, or mute the thread https://github.com/notifications/unsubscribe-auth/AAZ0fjHNHSYzP4H-gHEhO38-U6SN6Nnzks5soj8-gaJpZM4PRnIB .