Skip to content
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

Remove custom retry map for exponential retries. #2688

Closed
adamsilverstein opened this issue Jan 26, 2021 · 6 comments
Closed

Remove custom retry map for exponential retries. #2688

adamsilverstein opened this issue Jan 26, 2021 · 6 comments
Labels
Good First Issue Good first issue for new engineers P1 Medium priority QA: Eng Requires specialized QA by an engineer Rollover Issues which role over to the next sprint Type: Enhancement Improvement of an existing feature
Milestone

Comments

@adamsilverstein
Copy link
Collaborator

adamsilverstein commented Jan 26, 2021

Bug Description

In #2442 / b1b73c9 we added a custom retry map for exponential retries. The main reason we added this was to avoid retrying for lightouseErrors which can return a 500 code even when not retryable.

Since that time, I opened a PR to add this config to the upstream library which has now been merged: googleapis/google-api-php-client#2010. The client config now matches ours, so we can remove our custom config:

https://github.com/adamsilverstein/google-api-php-client/blob/b4aadd3a71213a19a9076af90a1cbef85eec8a83/src/Task/Runner.php#L75-L86

Steps to reproduce

  1. Go to '...'
  2. Click on '....'
  3. Scroll down to '....'
  4. See error

Screenshots

Additional Context

  • PHP Version:
  • OS: [e.g. iOS]
  • Browser [e.g. chrome, safari]
  • Plugin Version [e.g. 22]
  • Device: [e.g. iPhone6]

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • The exponential retry feature should use the default configuration.
  • The PHP API client should be running at least version 2.9.0 (when the upstream change was released)

Implementation Brief

  • In /includes/Core/Authentication/Clients/OAuth_Client.php:
    • Remove the retry map array.
    • Remove the line setting the retry config: $client->setConfig( 'retry_map', $this->retry_map );
  • In composer.json update the version for google/apiclient to the latest released version. (currently 2.9.1).

Test Coverage

  • No change.

Visual Regression Changes

  • No change.

QA Brief

  • Engineer to confirm the retry map has been removed.

Changelog entry

  • Update Google API client library and remove custom configuration to retry failed API requests as it is now covered in the library itself.
@adamsilverstein adamsilverstein added Type: Bug Something isn't working Good First Issue Good first issue for new engineers labels Jan 26, 2021
@felixarntz felixarntz added Next Up P1 Medium priority Type: Enhancement Improvement of an existing feature and removed Type: Bug Something isn't working labels Jan 26, 2021
@felixarntz
Copy link
Member

Noting here that we'll need to require at least version 2.9.0 of the google-api-php-client to make this change.

@felixarntz
Copy link
Member

@adamsilverstein Can you update the ACs and IB based on the above? Afterwards should be good.

@adamsilverstein
Copy link
Collaborator Author

👍🏼 Updated.

@felixarntz felixarntz self-assigned this Jan 26, 2021
@felixarntz
Copy link
Member

IB ✅

@felixarntz felixarntz removed their assignment Jan 26, 2021
@fhollis fhollis added this to the Sprint 42 milestone Jan 28, 2021
@fhollis fhollis removed the Next Up label Feb 1, 2021
@benbowler benbowler self-assigned this Feb 8, 2021
@benbowler benbowler added the QA: Eng Requires specialized QA by an engineer label Feb 8, 2021
@benbowler
Copy link
Collaborator

Added a PR and QA:Eng line.

@benbowler benbowler removed their assignment Feb 8, 2021
@eugene-manuilov eugene-manuilov removed their assignment Feb 11, 2021
@fhollis fhollis removed this from the Sprint 42 milestone Feb 15, 2021
@fhollis fhollis added this to the Sprint 43 milestone Feb 15, 2021
@fhollis fhollis added the Rollover Issues which role over to the next sprint label Feb 15, 2021
@tofumatt tofumatt self-assigned this Feb 17, 2021
@tofumatt
Copy link
Collaborator

QA ✅

@tofumatt tofumatt removed their assignment Feb 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue Good first issue for new engineers P1 Medium priority QA: Eng Requires specialized QA by an engineer Rollover Issues which role over to the next sprint Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

6 participants