8000 Update gax -> 1.47.0, guava -> 28.0-android, gson -> 2.8.5 by chingor13 · Pull Request #5590 · googleapis/google-cloud-java · GitHub
[go: up one dir, main page]
Skip to content

Update gax -> 1.47.0, guava -> 28.0-android, gson -> 2.8.5 #5590

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.

Sign up for GitHub

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

Jump to bottom
Merged
chingor13 merged 1 commit into from
Jul 1, 2019
Merged

Update gax -> 1.47.0, guava -> 28.0-android, gson -> 2.8.5 #5590

chingor13 merged 1 commit into from
Jul 1, 2019

Conversation

@chingor13
Copy link
Contributor
@chingor13 chingor13 commented Jun 28, 2019

Fixes #5555

@chingor13 chingor13 requested a review from a team as a code owner June 28, 2019 16:21
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 28, 2019
@chingor13 chingor13 mentioned this pull request Jun 28, 2019
Closed
1 task
@chingor13
Copy link
Contributor Author
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
Copy link
Contributor
kolea2 commented Jun 29, 2019

@olavloite can you weigh in here?

@olavloite
Copy link
olavloite commented Jul 1, 2019

@kolea2 @chingor13 @snehashah16
The Spanner client library used to disable both gax retries and timeouts by setting all retryable codes on the gapic rpc client to an empty set, and then handle all retries and timeouts itself. I added these test cases during the transition from the custom retry handling to using the default gax retry logic in order to verify the behavior in both the old and new situation. I guess we don't need the test cases for the old situation anymore, as the transition has been completed.

@olavloite
Copy link
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 executeStreamingSql method of Spanner. In gax 1.46.1 and earlier, setting this did not cause the method to throw a DEADLINE_EXCEEDED exception. In Gax 1.47 it does (when the execution time exceeds the timeout settings).

final RetrySettings retrySettings =
    RetrySettings.newBuilder()
        .setInitialRpcTimeout(Duration.ofMillis(500L))
        .setMaxRpcTimeout(Duration.ofMillis(500L))
        .setMaxAttempts(3)
        .setTotalTimeout(Duration.ofMillis(1500L))
        .build();
SpannerOptions.Builder builder =
    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 (ResultSet rs = client.singleUse().executeQuery(SELECT1AND2)) {
  while (rs.next()) {}
}

@olavloite
Copy link
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.

fix-spanner-test-cases.zip

@sduskis
Copy link
Contributor
sduskis commented Jul 1, 2019

@olavloite, can you please submit a separate PR for those changes?

@olavloite
Copy link
olavloite commented Jul 1, 2019

@sduskis PR for this change can be found here: #5625

I've (temporarily) disabled the test case for executeStreamingSql as that test case would fail in gax 1.46.

@sduskis sduskis added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 1, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 1, 2019
@codecov
Copy link
codecov bot commented Jul 1, 2019 •
edited
Loading

Codecov Report

Merging #5590 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            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     8918

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 1f05725...5fbaa37. Read the comment docs.

@chingor13 chingor13 merged commit d8b15d1 into googleapis:master Jul 1, 2019
@chingor13 chingor13 deleted the update-gax branch July 1, 2019 19:49
This was referenced Jul 2, 2019
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@kolea2 kolea2 kolea2 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

New Cloud Tasks Library has breaking changes

6 participants

@chingor13 @kolea2 @olavloite @sduskis @googlebot @yoshi-kokoro
0