Skip to content

fix: Enabled automatic HTTP retries for FirebaseProjectManagement #356

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

Merged
merged 6 commits into from
Feb 10, 2020

Conversation

hiranya911
Copy link
Contributor

@hiranya911 hiranya911 commented Jan 28, 2020

Enables automatic HTTP retries for the FirebaseProjectManagement API. We disable retries for unit tests for faster execution of tests.

RELEASE NOTE: Project management APIs in the FirebaseProjectManagement class now automatically retries operations that fail due to retry-eligible HTTP errors.

@hiranya911 hiranya911 removed their assignment Jan 30, 2020

// Verify retry support
HttpUnsuccessfulResponseHandler retryHandler = request.getUnsuccessfulResponseHandler();
assertTrue(retryHandler instanceof RetryHandlerDecorator);
Copy link
Contributor

@weixifan weixifan Feb 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first I kind of want to say we should bake this assertion into the API infrastructure itself instead of directly verifying in a unit test that no one touched the code. But then the OO design I came up with to facilitate this turns out to be impossible due to various classes being final. I was hoping that we can write a subclass of HttpRequest called RetryingHttpRequest that declares at compile time that getUnsuccessfulResponseHandler will return RetryHandlerDecorator or a suitable base class, but alas, HttpRequest is final. (And so is HttpRequestFactory, which is the other class I wanted to subclass.)

Without the above OO structure, the only other way to structure the test to be more behavioural would be to treat the retrying mechanism completely like a black box, and empirically measure retry intervals and retry counts after forcing an unsuccessful response. This is going to be painfully flaky, so I think what you have is probably for the best.

Copy link
Contributor

@weixifan weixifan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM. The only comment I left is just an interesting observation.

@hiranya911 hiranya911 assigned hiranya911 and unassigned weixifan Feb 10, 2020
@hiranya911 hiranya911 changed the title Enabled automatic HTTP retries for FirebaseProjectManagement fix: Enabled automatic HTTP retries for FirebaseProjectManagement Feb 10, 2020
@hiranya911 hiranya911 merged commit a039e05 into master Feb 10, 2020
@hiranya911 hiranya911 deleted the hkj-project-mgt-retry branch February 10, 2020 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants