fix: allows retries when setting maxAttempts without setting TotalTimeout

[rahulKQL]

Fixes #827

[codecov]

Codecov Report

Merging #835 into master will decrease coverage by 0.1%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master     #835      +/-   ##
============================================
- Coverage     78.72%   78.61%   -0.11%     
+ Complexity     1146     1143       -3     
============================================
  Files           202      202              
  Lines          5099     4517     -582     
  Branches        405      374      -31     
============================================
- Hits           4014     3551     -463     
+ Misses          912      794     -118     
+ Partials        173      172       -1

Impacted Files	Coverage Δ	Complexity Δ
...le/api/gax/retrying/ExponentialRetryAlgorithm.java	100% <100%> (+2.43%)	12 <0> (+2)	⬆️
.../com/google/api/gax/batching/BatchingSettings.java	66.66% <0%> (-19.05%)	2% <0%> (ø)
...ava/com/google/api/gax/retrying/RetrySettings.java	15.38% <0%> (-15.87%)	2% <0%> (ø)
...m/google/api/gax/batching/FlowControlSettings.java	66.66% <0%> (-11.91%)	2% <0%> (ø)
...om/google/longrunning/stub/GrpcOperationsStub.java	77.77% <0%> (-11.88%)	10% <0%> (-1%)
...ain/java/com/google/api/gax/rpc/ClientContext.java	71.79% <0%> (-10.03%)	8% <0%> (ø)
...google/api/gax/httpjson/GaxHttpJsonProperties.java	40% <0%> (-10%)	2% <0%> (ø)
...m/google/api/gax/longrunning/OperationFutures.java	50% <0%> (-9.1%)	1% <0%> (ø)
.../grpc/GrpcServerStreamingRequestParamCallable.java	71.42% <0%> (-8.58%)	2% <0%> (ø)
...le/api/gax/core/InstantiatingExecutorProvider.java	50% <0%> (-7.15%)	3% <0%> (ø)
... and 83 more

---

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 68ae22d...b4b2236. Read the comment docs.

[igorbernstein2]

LGTM!

@vam-google, can you take a look as well?

[vam-google]

Looking at the original issue it is not clear why the decision was made to modify the algorithm itself but not how the retry settings builder behaves.

Also, this looks like backward incompatible behavior change for a GA component.

[codecov-io]

Codecov Report

Merging #835 into master will increase coverage by 0.01%.
The diff coverage is 83.33%.

@@             Coverage Diff              @@
##             master     #835      +/-   ##
============================================
+ Coverage     78.72%   78.73%   +0.01%     
- Complexity     1146     1151       +5     
============================================
  Files           202      202              
  Lines          5099     5107       +8     
  Branches        405      407       +2     
============================================
+ Hits           4014     4021       +7     
- Misses          912      913       +1     
  Partials        173      173

Impacted Files	Coverage Δ	Complexity Δ
...ava/com/google/api/gax/retrying/RetrySettings.java	31.25% <ø> (ø)	2 <0> (ø)	⬇️
...rc/main/java/com/google/api/gax/rpc/Callables.java	68.96% <100%> (+1.1%)	14 <5> (+2)	⬆️
...le/api/gax/retrying/ExponentialRetryAlgorithm.java	95.74% <77.77%> (-1.82%)	13 <0> (+3)

---

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 68ae22d...4fd4320. Read the comment docs.

[igorbernstein2]

Looking at the original issue it is not clear why the decision was made to modify the algorithm itself but not how the retry settings builder behaves.

I think the underlying issue is that the algorithm assumes that there is always a total timeout, which is why it needs to be updated

[vam-google]

@igorbernstein2 @rahulKQL I think if the case when both maxAttempts == 0 & totalTimeout == 0 would mean that the retries are disabled then I would be Ok with this PR. Also please update documentation for retry settings to explain that situation.

[igorbernstein2]

@rahulKQL can you update the RetrySettings docs? I think updating this this paragraph from

 * <p>The "total timeout" parameter has ultimate control over how long the logic should keep trying
 * the remote call until it gives up completely. The higher the total timeout, the more retries can
 * be attempted. The other settings are considered more advanced.

To:

 * <p>The "total timeout" and "max attempts" settings have ultimate control over how long the logic should keep trying the remote call until it gives up completely. The remote call will be retried until one of those thresholds is crossed. To avoid unbounded rpc calls, it is required to configure one of those settings. If both are 0, then retries will be disabled. The other settings are considered more advanced.

[igorbernstein2]

@rahulKQL can you also update the definition of

gax-java/gax/src/main/java/com/google/api/gax/rpc/Callables.java

Lines 249 to 251 in 8d7aafc

	private static boolean areRetriesDisabled(
	Collection<StatusCode.Code> retryableCodes, RetrySettings retrySettings) {
	return retrySettings.getMaxAttempts() == 1 || retryableCodes.isEmpty();

?

[rahulKQL]

@igorbernstein2 @vam-google Thanks for the review, I have updated this change request accordingly. Could you please have a fresh look!

[igorbernstein2]

LGTM