Skip to content

Pub/Sub: fix default RPC timeouts in publish retry settings#6257

Merged
anguillanneuf merged 3 commits intomasterfrom
pubsub
Sep 12, 2019
Merged

Pub/Sub: fix default RPC timeouts in publish retry settings#6257
anguillanneuf merged 3 commits intomasterfrom
pubsub

Conversation

@anguillanneuf
Copy link
Contributor

@anguillanneuf anguillanneuf commented Sep 11, 2019 •
edited
Loading

Setting initialRpcTimeout == maxRpcTimeout == totalTimeout effectively turns off publish retry.

google-cloud-java/google-cloud-clients/google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/v1/Publisher.java

Lines 453 to 462 in ff068a9

static final RetrySettings DEFAULT_RETRY_SETTINGS =
RetrySettings.newBuilder()
.setTotalTimeout(DEFAULT_TOTAL_TIMEOUT)
.setInitialRetryDelay(Duration.ofMillis(5))
.setRetryDelayMultiplier(2)
.setMaxRetryDelay(Duration.ofMillis(Long.MAX_VALUE))
.setInitialRpcTimeout(DEFAULT_RPC_TIMEOUT)
.setRpcTimeoutMultiplier(2)
.setMaxRpcTimeout(DEFAULT_RPC_TIMEOUT)
.build();

This PR updates the defaults.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 11, 2019
hongalex
hongalex approved these changes Sep 12, 2019
View reviewed changes
Copy link
Member

@hongalex hongalex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after the tests pass.

@codecov
Copy link

codecov bot commented Sep 12, 2019 •
edited
Loading

Codecov Report

Merging #6257 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #6257      +/-   ##
============================================
- Coverage      47.1%    47.1%   -0.01%     
+ Complexity    27376    27375       -1     
============================================
  Files          2524     2524              
  Lines        277599   277600       +1     
  Branches      31983    31983              
============================================
- Hits         130771   130767       -4     
- Misses       137054   137058       +4     
- Partials       9774     9775       +1
Impacted Files Coverage Δ Complexity Δ
...ain/java/com/google/cloud/pubsub/v1/Publisher.java 89.15% <100%> (+0.03%) 40 <0> (ø) ⬇️
.../cloud/datastore/testing/LocalDatastoreHelper.java 80.59% <0%> (-4.48%) 17% <0%> (ø)
...oogle/cloud/spanner/jdbc/SingleUseTransaction.java 86.5% <0%> (-1%) 36% <0%> (-1%)

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 55d95d2...aa86c8a. Read the comment docs.

@athakor athakor mentioned this pull request Sep 12, 2019
Closed
@pmakani pmakani added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 12, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 12, 2019
@anguillanneuf anguillanneuf merged commit 299f5bb into master Sep 12, 2019
@anguillanneuf anguillanneuf deleted the pubsub branch September 12, 2019 17:00
@chingor13 chingor13 mentioned this pull request Sep 18, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hongalex hongalex hongalex approved these changes

@sduskis sduskis Awaiting requested review from sduskis

+1 more reviewer

@kamalaboulhosn kamalaboulhosn kamalaboulhosn 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.

6 participants

@anguillanneuf @kamalaboulhosn @hongalex @googlebot @pmakani @yoshi-kokoro