-
Notifications
You must be signed in to change notification settings - Fork 145
Allow websocket retries with backoff time in between #341
Conversation
|
||
# Errors unrelated to websocket connection, not worth retrying. | ||
except (WebsocketIBMQProtocolError, WebsocketTimeoutError) as ex: | ||
raise ex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to except
the errors if you're just going to raise it? Or are you trying to document the possible exceptions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree! I think it would be much cleaner to simply remove them, since they are going to be raised regardless.
…iskit-ibmq-provider into retry-websocket-backoff
@@ -101,6 +104,8 @@ def _connect(self, url: str) -> Generator[Any, None, Connect]: | |||
was established, but the authentication failed. | |||
WebsocketIBMQProtocolError: if the connection to the websocket | |||
server was established, but the answer was unexpected. | |||
SSLError: if the connection to the websocket server could not | |||
be established due to SSL certificate errors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - an SSLError
might not be related to certificates; it could be other SSL errors. Also I don't know if you really need to document the SSLError
, since exceptions thrown by the underlying services are generally not documented. For example, websocket.send()
on line 131 can throw exceptions that are not caught nor documented.
|
||
def __init__(self, websocket_url: str, access_token: str) -> None: | ||
self.websocket_url = websocket_url.rstrip('/') | ||
self.access_token = access_token | ||
|
||
@asyncio.coroutine | ||
def _connect(self, url: str) -> Generator[Any, None, Connect]: | ||
def _connect(self, url: str) -> Generator[Any, None, WebSocketClientProtocol]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also update the docstring for the return type (from Connect
to WebSocketClientProtocol
)?
Summary
Fixes #331
Details and comments
The
WebsocketClient
class methodget_job_status
currently allows retrying once in the case of a connection closing. This PR allows the method to retry up to five times, with time in between.