Skip to content

chore: cleanup usage of runWithRetries#1017

Merged
BenWhitehead merged 3 commits intogoogleapis:masterfrom
BenWhitehead:StorageImpl-cleanup
Sep 15, 2021
Merged

chore: cleanup usage of runWithRetries#1017
BenWhitehead merged 3 commits intogoogleapis:masterfrom
BenWhitehead:StorageImpl-cleanup

Conversation

@BenWhitehead
Copy link
Copy Markdown
Collaborator

  • Cleanup and deduplicate error handling/exception translation logic
  • Condense all callables to lambdas

@product-auto-label product-auto-label bot added the api: storage Issues related to the googleapis/java-storage API. label Sep 14, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Sep 14, 2021
* Cleanup and deduplicate error handling/exception translation logic
* Condense all callables to lambdas
@BenWhitehead BenWhitehead marked this pull request as ready for review September 15, 2021 18:19
@BenWhitehead BenWhitehead requested a review from a team September 15, 2021 18:19
Copy link
Copy Markdown
Contributor

@tritone tritone left a comment

Choose a reason for hiding this comment

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

Love how much boilerplate this approach allows us to remove! Since it's a bit complicated as far as logic, I'd like to get a review from the java folks on the team as well (@JesseLovelace / @frankyn ).

Also, can you discuss how you tested this? I'm assuming that you used the conformance tests draft to guarantee that you are avoiding regressions.

Copy link
Copy Markdown
Contributor

@frankyn frankyn left a comment

Choose a reason for hiding this comment

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

Refactor LGTM, thanks for adding the additional javadoc.

There are more runWithRetries uses, could you clarify scope? It would be awesome if you could file a tracking issue to point back to as you continue refactoring.

@BenWhitehead
Copy link
Copy Markdown
Collaborator Author

Related to #1024

@BenWhitehead
Copy link
Copy Markdown
Collaborator Author

I created #1024 to track cleaning up the remaining usages

@BenWhitehead
Copy link
Copy Markdown
Collaborator Author

Related to the question of testing @tritone, this is a mainly syntactic refactor because we now have access to lambdas with java 8. Secondarily, because the lambdas are more terse, I also did the extra step of creating Retrying.run to remove the duplicated error handling, retry settings and api clock resolution that was everywhere.

@BenWhitehead BenWhitehead merged commit 5bbc977 into googleapis:master Sep 15, 2021
@BenWhitehead BenWhitehead deleted the StorageImpl-cleanup branch September 15, 2021 22:10
@BenWhitehead BenWhitehead added this to the Retry Conformance Project milestone Oct 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: storage Issues related to the googleapis/java-storage API. cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants