[1LP][RFR] added retry functionality in order to sort out Unathorized error in sprout #253
Conversation
dajoRH
changed the title
izapolsk
changed the title
| Sometimes connection to openshift api endpoint gets expired and openshift returns 401. | ||
| As a result tasks in some applications like sprout fail. | ||
| """ | ||
| def wrap(*args, **kwargs): |
There was a problem hiding this comment.
required a wrapper should use the functools.wraps decorator if possible
| return wrap | ||
|
|
||
|
|
||
| @reconnect(unauthenticated_error_handler) |
There was a problem hiding this comment.
form maintenance it might be better to actually apply the decorator on each method
There was a problem hiding this comment.
It isn't difficult to me to do that. however, there are very many methods and their amount will grow. Do you think it is necessary ?
izapolsk
force-pushed
the
expired_conn
branch
2 times, most recently
from
637ec81 to
cc77fa9
Compare
Unauthorized error
There was a problem hiding this comment.
If self.new_client is False, your reconnect logic won't do anything.
Probably best to have a 'force_new_client' kwarg on the _connect method, which your decorator will set to True when it does a 401 retry.
But do we even need this new_client kwarg? When do we ever want new_client to be False?
| try: | ||
| return method(*args, **kwargs) | ||
| except ApiException as e: | ||
| if e.reason == 'Unauthorized' and attempt <= attempts: |
There was a problem hiding this comment.
You can check for e.status == 401 here instead of looking at the string. May be more reliable.
There was a problem hiding this comment.
I decided to use reason instead of code because it gives clear understanding about the reason. Whilst using code will require additional comment.
| @wraps(method) | ||
| def wrap(*args, **kwargs): | ||
| attempts = 3 | ||
| for attempt in range(1, attempts + 1): |
There was a problem hiding this comment.
Checking for "attempt <= attempts" is redundant since it's impossible for attempt to ever be > attempts inside your for loop. I think this could be written as:
def wrap(*args, **kwargs):
reconnect_attempts = 3
for attempt in range(reconnect_attempts):
try:
return method(*args, **kwargs)
except ApiException as e:
if e.status == 401:
args[0]._connect()
# Final attempt, 401 exception won't be caught here if we still hit it.
return method(*args, **kwargs)That would give 4 total method attempts, with 3 reconnect attempts.
There was a problem hiding this comment.
I had the same code in the beginning but changed it to what you saw in order to make exact number of reconnect attempts. But it seems it isn't necessary :)
bsquizz
changed the title
No description provided.