Removed sseclient dependency + major enhancements on event_stream() #143
Conversation
| while servers and messages is None: | ||
| server = servers.pop(0) | ||
| def _do_sse_request(self, path): | ||
| for server in list(self.servers): |
There was a problem hiding this comment.
this is a change in behaviour. Previously, we were looping so long as we didn't have a response, but now we're doing the request on each server, irrespective of whether the request was successful. Please change it back :)
There was a problem hiding this comment.
Okie dokie !
There was a problem hiding this comment.
Sorry for the delay. I had so solve some personal problems on the last couple of days but I'm still going to fix this !
|
Can you address the failing itests? |
|
Working on it ! On Aug 23, 2016 17:32, "Kyle Anderson" [email protected] wrote:
|
| except Exception as e: | ||
| marathon.log.error('Error while calling %s: %s', url, e.message) | ||
|
|
||
| if messages is None: | ||
| raise MarathonError('No remaining Marathon servers to try') | ||
| if response.ok: |
There was a problem hiding this comment.
@Rob-Johnson This should fix, right ?
|
I do like the reduction in dependencies, but do we not need any SSE specific code that came with that client? We at Yelp don't use this feature. I'm only slightly more hesitant to merge it. @nuclon do you have any opinion? |
|
@solarkennedy If you want I can leave the sseclient dependency for now, not a problem but I searched the codebase and only found use on the _do_sse_request. See the previous code: def _do_sse_request(self, path, params=None, data=None):
from sseclient import SSEClient
headers = {'Accept': 'text/event-stream'}
messages = None
servers = list(self.servers)
while servers and messages is None:
server = servers.pop(0)
url = ''.join([server.rstrip('/'), path])
try:
messages = SSEClient(url, params=params, data=data, headers=headers,
auth=self.auth)
except Exception as e:
marathon.log.error('Error while calling %s: %s', url, e.message)
if messages is None:
raise MarathonError('No remaining Marathon servers to try')
return messagesThe sseclient is only imported on this very method. I'm not pushing but I think this is kinda safe. Feel free to ask any question. |
|
@solarkennedy I've made small pull request to sseclient recently and it fixes that weird CPU problem. Of course it's your call to decide - remove the dependency or not. |
|
@nuclon Instead of iterating char by char you made the iteration by line, am I right ? If so, that should do. def __next__(self):
while not self._event_complete():
try:
- nextchar = next(self.resp_iterator)
- self.buf += nextchar
- except (StopIteration, requests.RequestException):
+ nextline = self.resp_file.readline()
+ if not nextline:
+ raise EOFError()
+ self.buf += nextline
+ except (StopIteration, requests.RequestException, EOFError) as e:
time.sleep(self.retry / 1000.0)
self._connect()I would like (as a bonus of solving this problem) removing an external dependency but the truth is both codes would solve the issue with huge json responses from marathon. @solarkennedy Feel free to chose any. I just like to solve this problem asap. I have a running code on production that might snap at any time =/ |
|
I agree that removing the dep is good too. I'll make a new release to include this change. |
|
Hooray ! |
|
seeclient is still marked in setup.py as a dependency: https://github.com/thefactory/marathon-python/blob/master/setup.py#L16 |
|
I saw that after the merge was made. I ll make another pr to remove it from On Oct 17, 2016 11:19, "David Moreau Simard" [email protected]
|
No description provided.