damoxc commented 5 years ago

I've tried to implement this in a backwards compatible way, without adjusting the return value from API calls, so using a dictionary to essentially pass the index parameter by reference rather than value making it possible to update.

Interested in your thoughts on it.

codecov-io commented 5 years ago

Codecov Report

Merging #89 into master will decrease coverage by 0.14%. The diff coverage is 96.49%.

@@            Coverage Diff             @@
##           master      #89      +/-   ##
==========================================
- Coverage   91.23%   91.09%   -0.15%     
==========================================
  Files          26       26              
  Lines        1038     1044       +6     
==========================================
+ Hits          947      951       +4     
- Misses         91       93       +2

Impacted Files	Coverage Δ
nomad/api/nodes.py	`88.37% <100%> (ø)`	:arrow_up:
nomad/api/evaluation.py	`100% <100%> (ø)`	:arrow_up:
nomad/api/namespaces.py	`57.5% <100%> (ø)`	:arrow_up:
nomad/api/allocations.py	`100% <100%> (ø)`	:arrow_up:
nomad/api/jobs.py	`89.58% <100%> (ø)`	:arrow_up:
nomad/api/sentinel.py	`85.71% <100%> (ø)`	:arrow_up:
nomad/api/deployment.py	`93.33% <100%> (ø)`	:arrow_up:
nomad/api/node.py	`84.61% <100%> (ø)`	:arrow_up:
nomad/api/evaluations.py	`92.3% <100%> (ø)`	:arrow_up:
nomad/api/allocation.py	`100% <100%> (ø)`	:arrow_up:
... and 5 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 03557e3...ff27bc9. Read the comment docs.

damoxc commented 5 years ago

Regarding the timeouts, the blocking endpoints also support a wait parameter which is a Golang parseable duration . Docs are:

In addition to index, endpoints that support blocking will also honor a wait parameter specifying a maximum duration for the blocking request. This is limited to 10 minutes. If not set, the wait time defaults to 5 minutes. This value can be specified in the form of "10s" or "5m" (i.e., 10 seconds or 5 minutes, respectively). A small random amount of additional wait time is added to the supplied maximum wait time to spread out the wake up time of any concurrent requests. This adds up to wait / 16 additional time to the maximum duration.

So I think from our side we would want to enforce the blocking_timeout to be at least wait + (wait / 16) to avoid requests raising a connection timeout. Feels like we should support wait for completeness.

Testing wise I imagine the only thing possible to test would be that we correctly calculate the blocking_timeout, from my understanding when a blocking request returns it may or may not have changes making it tricky to assert something. If it's possible to start a blocking call then alter the object in question via another client instance we could then check our blocking client also returns?

jonathanrcross commented 5 years ago

Hey @damoxc, so I was trying around the wait querystring parameter against the Nomad API. It doesn't look like it the input is enforced.

I can give it a number beyond its limit of 10m, but will still return at the 10 minute mark. I'm not sure if we need to enforce that detail other than just providing the wait parameter for the constructor. Also sorry for the delayed responses i'll try to be better about response time.

jrxFive / python-nomad

RFC: Blocking queries index param #89

Codecov Report