fix: first attempt should use the min of RPC timeout and total timeout

[mutianf]

Thank you for opening a Pull Request! For general contributing guidelines, please refer to contributing guide

Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

• Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
• Ensure the tests and linter pass
• Code coverage does not decrease (if any source code was changed)
• Appropriate docs were updated (if necessary)

Fixes #2643 ☕️

[alicejli]

drive-by comment - I think this change intuitively makes sense, but we should make sure we document it properly as RetrySettings have caused a lot of confusion in the past. @lqiu96 has been working on a guide here: googleapis/google-cloud-java#10598

[lqiu96]

Thanks @mutianf. Can you create an issue to describe the issue that you and BigTable are facing so that we can have a bit more context?

[lqiu96]

I believe this check is needed because it's possible that totalTimeout is not set (or set to 0) and it's being limited by number of attempts? If that's correct, could you add a small comment above to detail this?

[mutianf]

Done! Also created a issue #2643

[lqiu96]

qq, do you know if there is a valid use case where the total timeout is non-zero (explicitly set by the user), but the the total timeout is set to be less than the initialRPCTimeout? The only thing that I can think of right now is that a user has both time and attempt bounds and they care more about the attempt bounds (maybe?). Even if that is the case, the initialRPCTimeout shouldn't be larger than the totalTimeout.

After some more thought, I'm slightly leaning towards having a check that ensures that the initialRPCTimeout is less than or equal to the total timeout if totalTimeout is set. It should eliminate the need for a check here. Something here:

sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/retrying/RetrySettings.java

Lines 429 to 452 in e542af8

	if (params.getTotalTimeout().toMillis() < 0) {
	throw new IllegalStateException("total timeout must not be negative");
	}
	if (params.getInitialRetryDelay().toMillis() < 0) {
	throw new IllegalStateException("initial retry delay must not be negative");
	}
	if (params.getRetryDelayMultiplier() < 1.0) {
	throw new IllegalStateException("retry delay multiplier must be at least 1");
	}
	if (params.getMaxRetryDelay().compareTo(params.getInitialRetryDelay()) < 0) {
	throw new IllegalStateException("max retry delay must not be shorter than initial delay");
	}
	if (params.getMaxAttempts() < 0) {
	throw new IllegalStateException("max attempts must be non-negative");
	}
	if (params.getInitialRpcTimeout().toMillis() < 0) {
	throw new IllegalStateException("initial rpc timeout must not be negative");
	}
	if (params.getMaxRpcTimeout().compareTo(params.getInitialRpcTimeout()) < 0) {
	throw new IllegalStateException("max rpc timeout must not be shorter than initial timeout");
	}
	if (params.getRpcTimeoutMultiplier() < 1.0) {
	throw new IllegalStateException("rpc timeout multiplier must be at least 1");
	}

, but that may be a behavior breaking change.

CC: @blakeli0

[mutianf]

Yeah, we have a customer that only sets total timeout to be 400ms. The default initialRpcTimeout in bigtable client is 20 seconds. I think adding the check here would cause the breakage for the customer.

[lqiu96]

Gotcha, I'm guessing they also either override the initialRpcTimeout or they're fine that call only runs a max of 400ms. I don't know the full context and there may be a use case where intialRpcTimeout > totalTimeout. Either way, I think you're right and we can't make that change. I think this solution is fine with me then.

[lqiu96]

/gcbrun

[lqiu96]

/gcbrun

[lqiu96]

@mutianf Changes LGTM and I think make sense. Please let @blakeli0 take a quick look and approve before merging. Thanks!

[blakeli0]

This fix makes sense but is also technically a change of behavior. If a customer has initialRpcTimeout set larger than totalTimeout, and has been incorrectly relying on this behavior, then they would have to change their code.
That being said, I think we should fix it but we should add some Javadoc to RetrySetting in relevant sections for totalTimeout and initialRpcTimeout. In case someone complains(hopefully not), we can at least point them to the relevant docs.

[mutianf]

This fix makes sense but is also technically a change of behavior. If a customer has initialRpcTimeout set larger than totalTimeout, and has been incorrectly relying on this behavior, then they would have to change their code. That being said, I think we should fix it but we should add some Javadoc to RetrySetting in relevant sections for totalTimeout and initialRpcTimeout. In case someone complains(hopefully not), we can at least point them to the relevant docs.

I think having initialRpcTimeout > totalTimeout is not really semantically meaningful. Since totalTimeout caps how long the entire operation will take. I'll add javadoc to RettrySettings.

[lqiu96]

/gcbrun

[lqiu96]

/gcbrun

[sonarcloud]

Quality Gate failed for 'gapic-generator-java-root'

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

[sonarcloud]

Quality Gate passed for 'java_showcase_integration_tests'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
88.9% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud