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

feat: add ability to partially update retry settings #993

Merged

Conversation

igorbernstein2
Copy link
Collaborator

@igorbernstein2 igorbernstein2 commented Mar 30, 2020

The current approach to updating retry settings is very error prone. We've had a few customers accidentally disable retries trying to update the total timeout doing something like:

SomeClientSettings.Builder clientSettingsBuilder = SomeClientSettings.newBuilder();
SomeClientStubSettings.Builder stubSettingsBuilder = clientSettingsBuilder.stubSettings();

stubSettingsBuilder.someMethod()
  .setRetrySettings(
     RetrySettings.newBuilder().setTotalTimeout(Duration.ofSecond(10)).build()
  );

The correct way is not really intuitive and quite verbose:

SomeClientSettings.Builder clientSettingsBuilder = SomeClientSettings.newBuilder();
SomeClientStubSettings.Builder stubSettingsBuilder = clientSettingsBuilder.stubSettings();

stubSettingsBuilder.someMethod()
  .setRetrySettings(
     stubSettingsBuilder.someMethod().getRetrySettings().toBuilder()
        .setTotalTimeout(Duration.ofSecond(10))
        .build()
  );

This PR exposes the underlying RetrySettings.Builder in Unary & ServerStreaming CallSettings. This allows the following pattern:

SomeClientSettings.Builder clientSettingsBuilder = SomeClientSettings.newBuilder();
SomeClientStubSettings.Builder stubSettingsBuilder = clientSettingsBuilder.stubSettings();

stubSettingsBuilder.someMethod()
  .addRetryableCode(Code.DEADLINE_EXCEEDED)
  .retrySettings()
     .setTotalTimeout(Duration.ofSecond(10));

@googlebot googlebot added the cla: yes label Mar 30, 2020
@codecov
Copy link

@codecov codecov bot commented Mar 30, 2020

Codecov Report

Merging #993 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #993      +/-   ##
============================================
+ Coverage     78.66%   78.67%   +<.01%     
  Complexity     1167     1167              
============================================
  Files           204      204              
  Lines          5152     5154       +2     
  Branches        413      413              
============================================
+ Hits           4053     4055       +2     
  Misses          925      925              
  Partials        174      174
Impacted Files Coverage Δ Complexity Δ
...java/com/google/api/gax/rpc/UnaryCallSettings.java 100% <100%> (ø) 5 <0> (ø) ⬇️
...oogle/api/gax/rpc/ServerStreamingCallSettings.java 67.85% <100%> (+0.58%) 6 <0> (ø) ⬇️

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 909bf1b...3d79f06. Read the comment docs.

Copy link
Contributor

@vam-google vam-google left a comment

In general I believe making nested builders is not salable and error-prone (each level of nested builders needs to propagate its "building" logic all way up to the bottom).

If this comes from the real users experience and really makes their life easier, then probably it is worth doing it. Please check the comments first.

Copy link
Contributor

@vam-google vam-google left a comment

LGTM

@igorbernstein2
Copy link
Collaborator Author

@igorbernstein2 igorbernstein2 commented Apr 1, 2020

Thanks for the review! Will merge after presubmits finish

@igorbernstein2 igorbernstein2 added the kokoro:force-run label Apr 2, 2020
@kokoro-team kokoro-team removed the kokoro:force-run label Apr 2, 2020
@igorbernstein2 igorbernstein2 added the kokoro:force-run label Apr 2, 2020
@kokoro-team kokoro-team removed the kokoro:force-run label Apr 2, 2020
@igorbernstein2 igorbernstein2 added the kokoro:force-run label Apr 2, 2020
@kokoro-team kokoro-team removed the kokoro:force-run label Apr 2, 2020
@igorbernstein2 igorbernstein2 added the kokoro:force-run label Apr 2, 2020
@igorbernstein2
Copy link
Collaborator Author

@igorbernstein2 igorbernstein2 commented Apr 2, 2020

@vam-google something weird is happening with kokoro...the integration, dependencies, format and binary compat presubmits never finish

@vam-google
Copy link
Contributor

@vam-google vam-google commented Apr 2, 2020

@chingor13 Do you have any ideas what is happening with kokoro?

@chingor13
Copy link
Contributor

@chingor13 chingor13 commented Apr 2, 2020

The setting configuration for the org got messed up and created extra required checks on repos it should have ignored. We should be able to remove those required checks, but I'm not an admin on this repo so I can't do it.

@chingor13 chingor13 merged commit e57452a into googleapis:master Apr 6, 2020
8 checks passed
@igorbernstein2 igorbernstein2 deleted the ergonomic-retry-settings branch Nov 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes kokoro:force-run
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants