Updated Watcher to use Bookmark events and restart watch from last observed event #94
Conversation
…om last observed event
|
I think it looks good, thanks for the contribution. I did a bit of testing locally, and then some reading of the API docs, and it seems it's expected that clients handle a "410 Gone" status from the API when sending the resourceVersion constraint in a query, as the server doesn't guarantee to know about old events: {"type":"ERROR","object":{"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"too old resource version: 1312242788 (1474332294)","reason":"Gone","code":410}}Catching that error and setting the last_seen_version back to None should work for this. I think it would be more friendly if the library took care of this, rather than leaving it up to clients |
|
/sem-approve |
|
☝️ Trying to get semaphore to build the PR... |
There was a problem hiding this comment.
Thanks for contributing. I have a few comments, nothing major. I also think the comments from @gregjones makes sense. Probably a good thing if the build goes green. 🙂
PS. Out of curiosity, how did you hear about this library, and what are you using it for? 🙂
| @@ -19,6 +19,7 @@ | |||
|
|
|||
| import json | |||
| import logging | |||
| from urllib.parse import urlencode, urlsplit, urlunsplit, parse_qs | |||
There was a problem hiding this comment.
We still support Python 2.7, so this fails. You can use six.moves to solve it: https://six.readthedocs.io/index.html?highlight=moves#module-six.moves
| @@ -73,11 +75,11 @@ def test_handle_changes(self, api_watch_list): | |||
| watcher._run_forever = False | |||
| assert list(gen) == [] | |||
|
|
|||
| def test_complicated(self, api_watch_list): | |||
| def test_many_reconnections(self, api_watch_list): | |||
There was a problem hiding this comment.
Good rename, easier to understand what we are actually testing here. 👍
| @@ -117,20 +118,38 @@ def list(cls, namespace="default"): | |||
| return [cls.from_dict(item) for item in resp.json()[u"items"]] | |||
|
|
|||
| @classmethod | |||
| def watch_list(cls, namespace=None): | |||
| def _watch_url_from_base_resource_url(cls, base_resource_url, start_at_resource_version=None): | |||
There was a problem hiding this comment.
Since the client.get call goes to requests.request in the end, passing any keyword arguments it gets, I think this whole thing could be replaced by simply passing a suitable params dictionary to the client.get call.
That would be much easier to work with, and easier to read than this method.
|
@gregjones Good point. I'll add handling for the "too old resource version" case. @mortenlj Indeed, I'll look at Semaphore and make the adjustments you suggested. I found your "hacktoberfest" tagged issues. I figured this was a good opportunity to learn more about the k8s API, as well as levelling up my Python skills. |
|
@MichelCarroll : Any movement on this? |
This PR is in response to #70 and #71.
I modified the
k8s.watchermodule to:Bookmarkevents without forwarding them to the listenerstart_at_resource_versionparameter)Please let me know what you think.