Propagate exceptions to the caller if Fis getToken call fails with an exception. #2035
Conversation
Binary Size ReportAffected SDKs
Test Logs
NotesHead commit (21054498) is created by Prow via merging commits: 9a8cd2c abf5e74. |
Coverage ReportAffected SDKs
Test Logs
NotesHTML coverage reports can be produced locally with Head commit (21054498) is created by Prow via merging commits: 9a8cd2c abf5e74. |
| @@ -42,8 +42,7 @@ public boolean onStateReached(PersistedInstallationEntry persistedInstallationEn | |||
| } | |||
|
|
|||
| @Override | |||
| public boolean onException( | |||
| PersistedInstallationEntry persistedInstallationEntry, Exception exception) { | |||
| public boolean onException(Exception exception) { | |||
| return false; | |||
There was a problem hiding this comment.
why don't we fail the completion source if an exception is triggered in the getId path?
There was a problem hiding this comment.
getId flow should never trigger to propagate exception because we immediately return an FID (when available or generating a new one) even before trying to register the FID with FIS servers.
…o fisExceptionFix
There was a problem hiding this comment.
PR2035
(to make sure I understand correctly)
What the change does is to always forward the exception to the caller in the #getToken path, even if the Installation is/was successfully registered with FIS.
So, what is an affected use-case here? Is this one?
An Installation was successfully registered with FIS servers, now one week later it tries to generate a new auth token, but the request fails due to network issues. Different to before, we will now forward these kind of errors to the caller.
Is that correct?
Yes, your understanding is right. |
…o fisExceptionFix
|
@ankitaj224: The following tests failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Background: YT reported a bug where FIS.getAuthTokens timed out https://b.corp.google.com/issues/168338136.
A bug was found in the FIS SDK during analysis of that bug where FIS SDK wasn't propagating an exception to the caller if FID was in a registered state & that resulted in getToken call to timeout.
Ideally: If FIS getToken processing resulted in an exception, it should be propagated to the caller.