8000 Pub/Sub: Update Publish Retry Settings in Sample by anguillanneuf · Pull Request #6258 · googleapis/google-cloud-java · GitHub
[go: up one dir, main page]
Skip to content

Pub/Sub: Update Publish Retry Settings in Sample #6258

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
anguillanneuf merged 4 commits into from
Sep 12, 2019
Merged

Pub/Sub: Update Publish Retry Settings in Sample #6258

anguillanneuf merged 4 commits into from
Sep 12, 2019

Conversation

@anguillanneuf
Copy link
Contributor
@anguillanneuf anguillanneuf commented Sep 12, 2019 •
edited
Loading

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

google-cloud-java/google-cloud-examples/src/main/java/com/google/cloud/examples/pubsub/snippets/PublisherSnippets.java

Lines 116 to 131 in 55d95d2

Duration retryDelay = Duration.ofMillis(5); // default: 5 ms
double retryDelayMultiplier = 2.0; // back off for repeated failures, default: 2.0
Duration maxRetryDelay = Duration.ofSeconds(600); // default : Long.MAX_VALUE
Duration totalTimeout = Duration.ofSeconds(10); // default: 10 seconds
Duration initialRpcTimeout = Duration.ofSeconds(10); // default: 10 seconds
Duration maxRpcTimeout = Duration.ofSeconds(10); // default: 10 seconds
RetrySettings retrySettings =
RetrySettings.newBuilder()
.setInitialRetryDelay(retryDelay)
.setRetryDelayMultiplier(retryDelayMultiplier)
.setMaxRetryDelay(maxRetryDelay)
.setTotalTimeout(totalTimeout)
.setInitialRpcTimeout(initialRpcTimeout)
.setMaxRpcTimeout(maxRpcTimeout)
.build();

This PR updates the defaults in the code sample and addresses #6254.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 12, 2019
@athakor athakor mentioned this pull request Sep 12, 2019
Closed
@kir-titievsky
Copy link
kir-titievsky commented Sep 12, 2019

Why 1.3 for retryDelayMultiplier? I don't have any particular reason why this is good or bad, just seems arbitrary.

@anguillanneuf
Copy link
Contributor Author
anguillanneuf commented Sep 12, 2019 •
edited
Loading

@kir-titievsky 1.3 does seem arbitrary. It comes from the client lib which we updated yesterday.

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

Lines 572 to 590 in 299f5bb

private static final Duration DEFAULT_INITIAL_RPC_TIMEOUT = Duration.ofSeconds(5);
private static final Duration DEFAULT_MAX_RPC_TIMEOUT = Duration.ofSeconds(600);
private static final Duration DEFAULT_TOTAL_TIMEOUT = Duration.ofSeconds(600);
static final BatchingSettings DEFAULT_BATCHING_SETTINGS =
BatchingSettings.newBuilder()
.setDelayThreshold(DEFAULT_DELAY_THRESHOLD)
.setRequestByteThreshold(DEFAULT_REQUEST_BYTES_THRESHOLD)
.setElementCountThreshold(DEFAULT_ELEMENT_COUNT_THRESHOLD)
.build();
static final RetrySettings DEFAULT_RETRY_SETTINGS =
RetrySettings.newBuilder()
.setTotalTimeout(DEFAULT_TOTAL_TIMEOUT)
.setInitialRetryDelay(Duration.ofMillis(100))
.setRetryDelayMultiplier(1.3)
.setMaxRetryDelay(Duration.ofSeconds(60))
.setInitialRpcTimeout(DEFAULT_INITIAL_RPC_TIMEOUT)
.setRpcTimeoutMultiplier(1)
.setMaxRpcTimeout(DEFAULT_MAX_RPC_TIMEOUT)
.build();

@codecov
Copy link
codecov bot commented Sep 12, 2019 •
edited
Loading

Codecov Report

Merging #6258 into master will increase coverage by 0.28%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #6258      +/-   ##
============================================
+ Coverage     46.81%    47.1%   +0.28%     
  Complexity    27375    27375              
============================================
  Files          2524     2524              
  Lines        277466   277600     +134     
  Branches      31147    31983     +836     
============================================
+ Hits         129907   130770     +863     
- Misses       136980   137055      +75     
+ Partials      10579     9775     -804
Impacted Files Coverage Δ Complexity Δ
...le/cloud/compute/deprecated/DeprecationStatus.java 88.63% <0%> (-3.13%) 20% <0%> (ø)
...le/cloud/storage/contrib/nio/CloudStoragePath.java 75.67% <0%> (-0.69%) 51% <0%> (ø)
...ge/contrib/nio/CloudStorageFileSystemProvider.java 62.76% <0%> (-0.3%) 73% <0%> (ø)
...ain/java/com/google/cloud/logging/LoggingImpl.java 87.9% <0%> (-0.19%) 73% <0%> (ø)
...m/google/cloud/spanner/jdbc/JdbcTypeConverter.java 74.86% <0%> (-0.14%) 88% <0%> (ø)
...oogle/cloud/spanner/jdbc/JdbcDatabaseMetaData.java 87.67% <0%> (-0.13%) 165% <0%> (ø)
...src/main/java/io/grafeas/v1/UpdateNoteRequest.java 11.74% <0%> (-0.08%) 11% <0%> (ø)
...in/java/io/grafeas/v1/UpdateOccurrenceRequest.java 11.71% <0%> (-0.08%) 11% <0%> (ø)
...ava/io/grafeas/v1/ListNoteOccurrencesResponse.java 14.28% <0%> (-0.05%) 10% <0%> (ø)
...in/java/io/grafeas/v1/ListOccurrencesResponse.java 14.28% <0%> (-0.05%) 10% <0%> (ø)
... and 173 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 299f5bb...5f22024. Read the comment docs.

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
< 8000 form class="js-comment-update" data-type="json" data-turbo="false" action="/googleapis/google-cloud-java/pull/6258/reviews/287677370/update" accept-charset="UTF-8" method="post">

@anguillanneuf anguillanneuf merged commit ffd34ee into master Sep 12, 2019
@anguillanneuf anguillanneuf deleted the pubsub_sample branch September 12, 2019 20:23
@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

@kamalaboulhosn kamalaboulhosn Awaiting requested review from kamalaboulhosn

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.

4 participants

@anguillanneuf @kir-titievsky @hongalex @googlebot
0