-
Notifications
You must be signed in to change notification settings - Fork 47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
improve token refresh logic #277
Conversation
_logger.info( | ||
"The newly acquired token on connection %r is the same as the previous one," | ||
" will keep attempting to refresh", | ||
self._connection.container_id |
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.
Is this log statement going to get very noisy? Or is it very rare that token == previous_token after calling update_token
?
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.
yes, it would be very noisy in the case token == previous_token
(which happens now when there's a gap between the client refresh window (uAMQP 6 mins to refresh) and service refresh window (AAD service 5 mins refresh))
So once we have fixed the issue in uamqp/eh/sb, changing default refresh window to 5 mins, it should be rare -- at least not worse than what we currently have.
I think what uamqp is doing technically is not wrong -- it's doing its best to refresh token, however it's doing a bit excessively, retry without backoff. So ultimately we might want to introduce kinda backoff to token refresh to let it not throttle.
with that being said, I think we could keep this "potential" noisy logging for now until user complains.
addressing issue: Azure/azure-sdk-for-python#20573
The improvements include:
TODO:
There's one more improvement I could think of, introducing backoff when calling "update_token" if the newly-generated token is the same as old one to avoid throttling token generation module/service, but I'm afraid that goes too far for now.