Uh oh!
There was an error while loading. Please reload this page.
Update gax -> 1.47.0, guava -> 28.0-android, gson -> 2.8.5 - #5590
Conversation
chingor13
commented
Jun 28, 2019
Spanner tests that disable gax retries are now failing. For non-retryable methods, we started setting a timeout from the retry config but the default setting is 10 minutes for API calls so it allowed these tests to pass. We "fixed" this for Tasks by preferring the initialRpcTimeout, then maxRpcTimeout, then totalTimeout for the default timeout, but this breaks the spanner tests. @kolea2@snehashah16 I'm not sure what the spanner tests that disable gax retries is attempting to do - if we're trying to check that disabling retries also disables timeouts, that's in direct conflict with what gax implemented in googleapis/gax-java#712. |
kolea2
commented
Jun 29, 2019
@olavloite can you weigh in here? |
olavloite
commented
Jul 1, 2019
@kolea2@chingor13@snehashah16 |
olavloite
commented
Jul 1, 2019
@chingor13 I'm changing the test cases to only test for the new situation, i.e. with Gax handling of retries and timeouts enabled. One other thing that I noticed while changing these test cases is that the retry handling of streaming calls have also changed. Can you confirm that that is correct? The following code sets a timeout for the finalRetrySettingsretrySettings =
RetrySettings.newBuilder()
.setInitialRpcTimeout(Duration.ofMillis(500L))
.setMaxRpcTimeout(Duration.ofMillis(500L))
.setMaxAttempts(3)
.setTotalTimeout(Duration.ofMillis(1500L))
.build();
SpannerOptions.Builderbuilder =
SpannerOptions.newBuilder()
.setProjectId("[PROJECT]")
.setChannelProvider(channelProvider)
.setCredentials(NoCredentials.getInstance());
builder
.getSpannerStubSettingsBuilder()
.executeStreamingSqlSettings()
.setRetrySettings(retrySettings);
...
// In gax 1.46.1 and earlier, this will succeed.// In gax 1.47, this will throw a DEADLINE_EXCEEDED.mockSpanner.setExecuteStreamingSqlExecutionTime(ONE_SECOND);
try (ResultSetrs = client.singleUse().executeQuery(SELECT1AND2)) {
while (rs.next()) {}
}
|
olavloite
commented
Jul 1, 2019
The enclosed patch should fix the broken test cases by removing all test cases with gax disabled. Also, the test case for executeStreamingSql has been updated to reflect the fact that it now does throw a DEADLINE_EXCEEDED when a timeout has been set. |
sduskis
commented
Jul 1, 2019
@olavloite, can you please submit a separate PR for those changes? |
olavloite
commented
Jul 1, 2019
Codecov Report
@@ Coverage Diff @@## master #5590 +/- ##
=========================================
Coverage 46.71% 46.71% Complexity 24627 24627 =========================================
Files 2351 2351 Lines 256125 256125 Branches 29325 29325 =========================================
Hits 119648 119648 Misses 127559 127559 Partials 8918 8918Continue to review full report at Codecov.
|
Fixes#5555