Bug/pla 3008 notebook connection reset error#253
Conversation
jfriend-citrine
left a comment
jfriend-citrine
left a comment
There was a problem hiding this comment.
LGTM
Let's find a resilience and transient-fault-handling library that allows developers to express policies such as Retry, Circuit Breaker, Timeout, Bulkhead Isolation, and Fallback.
| response = super().request(method, uri, **kwargs) | ||
| except (ConnectionError, ConnectionResetError): | ||
| logger.debug('Connection Error, creating a new session') | ||
| super().__init__() |
There was a problem hiding this comment.
What exactly does this do? Does this reset the state of the citrine session? How are the parameters that were used to instantiate the citrine session (e.g. scheme, host, api key) persisted?
There was a problem hiding this comment.
The citrine-python Session is a subclass of Requests.Session. The CP Session.init() calls the superclass's (Requests.Session's) init() with no arguments. I had a look as Request's Session.init() and this is where urllib3 connections are created.
All of the arguments passed to CP's Session.init() are left unchanged so they should persist as normal. I'm not sure which attributes are used by the superclass, e.g. refresh_token.
This PR doesn't try to refresh the token if one of these errors are seen. This is something we can look into if required.
This change has not been tested with Jupyter notebooks to see if this addresses the possible timeout issue.
There was a problem hiding this comment.
Got it. This sounds great to me. I'll merge.
3 participants
Citrine Python PR
Description
This PR is a proposed bugfix for https://citrine.atlassian.net/browse/PLA-3008. If a ConnectionResetError or ConnectionError exception is raised while running a request, this will reinitialize the session and try the request again. This does not use recursion.
I view this as a low risk PR as there is no substantial change to the logic.
PR Type:
Adherence to team decisions