Skip to content
This repository was archived by the owner on Sep 26, 2023. It is now read-only.

Conversation

@noahdietz
Copy link
Contributor

@noahdietz noahdietz commented Jul 16, 2020

This is a WIP PR that will deprecate the timeout-backoff implementation in ExponentialRetryAlgorithm. The first RPC in a logical call with have a deadline calculated as now() + totalTimeout. Should that first RPC fail with a retry-able status code, the deadline of the next attempted RPC will be calculated as now() + (totalTimeout - timeElapsed - retryDelay) where (totalTimeout - timeElapsed - retryDelay) is the amount of time left in the totalTimeout at the start of the next attempt after the retryDelay.

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jul 16, 2020
@codecov
Copy link

codecov bot commented Jul 16, 2020

Codecov Report

Merging #1152 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1152   +/-   ##
=========================================
  Coverage     79.07%   79.08%           
  Complexity     1193     1193           
=========================================
  Files           205      205           
  Lines          5258     5259    +1     
  Branches        433      433           
=========================================
+ Hits           4158     4159    +1     
  Misses          929      929           
  Partials        171      171           
Impacted Files Coverage Δ Complexity Δ
...le/api/gax/retrying/ExponentialRetryAlgorithm.java 95.91% <100.00%> (+0.08%) 13.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b7646a3...57aeda1. Read the comment docs.

Copy link
Contributor

@miraleung miraleung left a comment

Choose a reason for hiding this comment

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

Not enough context to confirm / approve, but I'd recommend adding some comments near the changed areas so that future readers understand what the algorithm is trying to do (and hopefully will not change it back).

Copy link
Contributor

@igorbernstein2 igorbernstein2 left a comment

Choose a reason for hiding this comment

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

This looks like a good direction. The only thing that I think needs to be addressed is what happens to users that have rpc deadline set in RetrySettings? I think this is particularly relevant for cases where retries are disabled.

Also I think that the rpc timeout methods on the RetrySettings should be marked as deprecated.

Finally should the overall timeout live in RetrySettings? If you are breaking compatibility, this might be a good time to move it elsewhere. It always bothered me that when retries are disabled, you still have to interact with RetrySettings to set a default deadline

@igorbernstein2
Copy link
Contributor

This looks good, but I think its currently missing a migration story. This is a behavioral change that could have negative repercussions for users. Some things to consider:

  • Deprecate rpc timeouts in RetrySettings
  • Consider rolling is out with feature flag where either client maintainers or end users can opt in
  • do an assessment of the current defaults used in google clients to see if how much of an affect this will have
  • ask client maintainers if this is acceptable

@noahdietz
Copy link
Contributor Author

Thanks @igorbernstein2 you're totally right. I am first confirming the idea with some other language leads and will come back to this PR to wrap up the deprecation/roll out items you've mentioned. I'm going to leave this as a draft until we've got the plan written down.

@noahdietz noahdietz force-pushed the deprecate-timeout-backoff branch from 7a2a16f to 57aeda1 Compare July 31, 2020 18:23
@noahdietz
Copy link
Contributor Author

Closing this in favor of a different implementation strategy.

@noahdietz noahdietz closed this Aug 12, 2020
@noahdietz noahdietz deleted the deprecate-timeout-backoff branch August 12, 2020 18:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

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