[Bigtable] Documented defaults timeout values in the EnhancedBigtableStubSettings #5126

rahulKQL · 2019-05-08T18:31:10Z

Updated default timeouts for all the operations in EnhnaceDBigtableStubSettings as BigtableClientSettings settings were deprecated.

Updated timeouts in `EnhnaceDBigtableStubSettings` as `BigtableClientSettings` settings methods are deprecated

rahulKQL · 2019-05-08T18:33:10Z

@igorbernstein2 Please have a look at this change.
Please let me know if the format can be improved for timeout listing.

...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubSettings.java

igorbernstein · 2019-05-14T23:23:11Z

@kennethlowery8190, please remove the approval. This pr needs more work and I don’t want it accidentally merged

- Listed Description for straming and non-streaming operation. - Tried explaning RPC vs Total timeout - Tried explaining retryDelayMultiplier. - Added default values on each setting operation's javadoc. - Added bulkMutateRowsSettings description.

codecov · 2019-05-31T16:18:35Z

Codecov Report

Merging #5126 into master will increase coverage by 0.48%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #5126      +/-   ##
============================================
+ Coverage     50.38%   50.87%   +0.48%     
- Complexity    23736    24223     +487     
============================================
  Files          2248     2271      +23     
  Lines        226471   230204    +3733     
  Branches      24954    25007      +53     
============================================
+ Hits         114109   117113    +3004     
- Misses       103770   104452     +682     
- Partials       8592     8639      +47

Impacted Files	Coverage Δ	Complexity Δ
...ble/data/v2/stub/EnhancedBigtableStubSettings.java	`95.94% <100%> (-2.67%)`	`32 <6> (+15)`
...ataproc/v1beta2/WorkflowTemplateServiceClient.java	`56.92% <0%> (-6.56%)`	`33% <0%> (ø)`
...n/java/com/google/cloud/bigquery/BigQueryImpl.java	`80% <0%> (-4.1%)`	`57% <0%> (+4%)`
...-core/src/main/java/com/google/cloud/Identity.java	`86.27% <0%> (-3.93%)`	`19% <0%> (-1%)`
...oud/dataproc/v1/WorkflowTemplateServiceClient.java	`56.92% <0%> (-3.57%)`	`33% <0%> (ø)`
...in/java/com/google/cloud/pubsub/v1/Subscriber.java	`77.52% <0%> (-3.54%)`	`17% <0%> (ø)`
...a/com/google/cloud/dialogflow/v2/AgentsClient.java	`47.72% <0%> (-2.88%)`	`19% <0%> (ø)`
.../google/cloud/dialogflow/v2beta1/AgentsClient.java	`47.72% <0%> (-2.88%)`	`19% <0%> (ø)`
...oogle/cloud/talent/v4beta1/JobServiceSettings.java	`13.72% <0%> (-2.56%)`	`2% <0%> (ø)`
... and 134 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 6879d9b...c467bea. Read the comment docs.

rahulKQL · 2019-05-31T16:19:44Z

@igorbernstein2 Please have another round of review.

I have added below points in the last commit.

Listed Description for streaming and non-streaming operation.
Tried explaining RPC vs Total timeout
Tried explaining retryDelayMultiplier.
Added default values on each setting operation's Javadoc.
Added bulkMutateRowsSettings description.

sduskis · 2019-06-11T21:02:40Z

@igorbernstein2 or @kolea2, can you PTAL

igorbernstein2

I added some suggestions for rewording the values. Let me know what you think

igorbernstein2 · 2019-06-11T21:14:26Z

...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubSettings.java

+ *   <li>Each operation setting accepts RPC and total time out. Here, RPC timeouts are for each
+ *       attempt to get the result whereas total timeout applies to the operation timeout.
+ *   <li>RetryDelayMultiplier controls the change in retry delay. After initial attempt, subsequent
+ *       attempts are calculated by multiplying the retry delay of the previous call.


This feels out of place. The first 3 bullet points describe the values of the settings, while new 3 points are describing concepts. Also, what are you referring to by this settings? Also, it's weird that the explanation for RetryDelayMultiplier is 3 bullet points away from the Retries bullet point.

I think it would be better to link users to read the docs for RetrySettings for a description of the concepts and focus on the values in these docs. (see below)

I think with your new suggested explanation, User will get the idea about these Retries and RPC timeouts so I have removed these explanations from the Class level Javadoc.
Please let me know if we want to add any details here.

...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubSettings.java

igorbernstein2 · 2019-06-11T21:27:08Z

...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubSettings.java

+   * Code#UNAVAILABLE} failure code.
+   *
+   * <p>RetryDelay starts with 100ms or 0.1 seconds, which multiplies with 1.3 times on every
+   * reattempt till a max retry timeout value of 60 seconds.


Please drop 0.1 seconds. Also, this isnt a timeout. Maybe reword to:

The RetryDelay between failed attempts starts with 100ms and exponentially increases by a factor of 1.3 until 60 seconds.

Thanks for the detailed suggestion, I have updated these settings with the same.

igorbernstein2 · 2019-06-11T21:29:19Z

...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubSettings.java

+   * <p>RetryDelay starts with 100ms or 0.1 seconds, which multiplies with 1.3 times on every
+   * reattempt till a max retry timeout value of 60 seconds.
+   *
+   * <p>RPC timeout is 20 seconds.


I think you should combine the 2 timeouts docs and call out that the RPC timeout means wait timeout:

The default read timeout for each row in a response stream is 20 seconds. And the timeout to read the entire stream is 1 hour.

igorbernstein2 · 2019-06-11T21:34:28Z

...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubSettings.java

+   *
+   * <p>RPC timeout is 20 seconds.
+   *
+   * <p>Total timeout for this operation is 600 seconds or 10 mins.


Similar to the streaming example I think you should combine the timeouts:

The default timeout for each attempt is 20 seconds and the timeout for the entire operation across all of the attempts is 10 mins.

igorbernstein2 · 2019-06-11T21:38:44Z

...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubSettings.java

+   *
+   * <p>This is idempotent and streaming operation.
+   *
+   * <p>The idle timeout is configured via {@code ServerStreamingCallSettings#getIdleTimeout}. This


Maybe nest these in a list? Also as opposed to duplicating the docs in RetrySettings and ServerStreamingSettings, just link out to them. Also I think it would better to link directly to the setters

Maybe something this?

<p>Default settings: <ul> <li>{@link ServerStreamingCallSettings.Builder#setIdleTimeout Default idle timeout} is set to 1 hour <li>{@link ServerStreamingCallSettings.Builder#setRetryableCodes Error codes} are: {@link Code#DEADLINE_EXCEEDED} and {@link Code#UNAVAILABLE} <li>RetryDelay {@link RetrySettings.Builder#setInitialRetryDelay starts} at 100ms and {@link RetrySettings.Builder#setRetryDelayMultiplier increases exponentially} by 1.3 until a maximum of ....

rahulKQL · 2019-06-13T20:30:35Z

@igorbernstein2 I tried to address feedback comments, Please take a look.

...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubSettings.java

igorbernstein2 · 2019-06-18T18:10:10Z

Just a couple minor nits, but LGTM!

rahulKQL · 2019-06-18T18:59:56Z

@igorbernstein2 Have addressed your feedback, Please have another look.

igorbernstein2

LGTM. Thanks!

igorbernstein2 · 2019-06-18T19:36:32Z

Look like a spanner test is failing

rahulKQL added 2 commits May 8, 2019 16:06

Documented defaults timeout values in the Bigtable Settings

00a69c7

Updated timeouts in `EnhnaceDBigtableStubSettings` as `BigtableClientSettings` settings methods are deprecated

javadoc fix

bdd4421

rahulKQL requested a review from a team as a code owner May 8, 2019 18:31

googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 8, 2019

igorbernstein2 suggested changes May 8, 2019

View reviewed changes

...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubSettings.java Outdated Show resolved Hide resolved

...table/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubSettings.java Outdated Show resolved Hide resolved

pmakani added api: bigtable Issues related to the Bigtable API. needs more info This issue needs more information from the customer to proceed. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels May 13, 2019

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 13, 2019

kennethlowery8190 approved these changes May 14, 2019

View reviewed changes

yoshi-automation added the 🚨 This issue needs some love. label May 15, 2019

sduskis added needs work This is a pull request that needs a little love. and removed 🚨 This issue needs some love. needs more info This issue needs more information from the customer to proceed. labels May 28, 2019

Addressing feedback comments

6ca6923

- Listed Description for straming and non-streaming operation. - Tried explaning RPC vs Total timeout - Tried explaining retryDelayMultiplier. - Added default values on each setting operation's javadoc. - Added bulkMutateRowsSettings description.

igorbernstein2 reviewed Jun 11, 2019

View reviewed changes

Updated Javadoc according to feedback

d62e034

sduskis added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 13, 2019

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 13, 2019

igorbernstein2 reviewed Jun 18, 2019

View reviewed changes

addressing feedback comments

c467bea

igorbernstein2 approved these changes Jun 18, 2019

View reviewed changes

kolea2 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 18, 2019

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 18, 2019

igorbernstein2 added kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed needs work This is a pull request that needs a little love. labels Jun 18, 2019

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 18, 2019

yoshi-automation added the 🚨 This issue needs some love. label Jun 18, 2019

igorbernstein2 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 19, 2019

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 19, 2019

igorbernstein2 merged commit acab5dc into googleapis:master Jun 19, 2019

kolea2 mentioned this pull request Jun 20, 2019

Release google-cloud-java v0.97.0 #5528

Merged

[Bigtable] Documented defaults timeout values in the EnhancedBigtableStubSettings #5126

[Bigtable] Documented defaults timeout values in the EnhancedBigtableStubSettings #5126

Uh oh!

Conversation

rahulKQL commented May 8, 2019

Uh oh!

rahulKQL commented May 8, 2019

Uh oh!

Uh oh!

Uh oh!

igorbernstein commented May 14, 2019

Uh oh!

codecov bot commented May 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rahulKQL commented May 31, 2019

Uh oh!

sduskis commented Jun 11, 2019

Uh oh!

igorbernstein2 left a comment

Choose a reason for hiding this comment

Uh oh!

igorbernstein2 Jun 11, 2019

Choose a reason for hiding this comment

Uh oh!

rahulKQL Jun 13, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

igorbernstein2 Jun 11, 2019

Choose a reason for hiding this comment

Uh oh!

rahulKQL Jun 13, 2019

Choose a reason for hiding this comment

Uh oh!

igorbernstein2 Jun 11, 2019

Choose a reason for hiding this comment

Uh oh!

igorbernstein2 Jun 11, 2019

Choose a reason for hiding this comment

Uh oh!

igorbernstein2 Jun 11, 2019

Choose a reason for hiding this comment

Uh oh!

rahulKQL commented Jun 13, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

igorbernstein2 commented Jun 18, 2019

Uh oh!

rahulKQL commented Jun 18, 2019

Uh oh!

igorbernstein2 left a comment

Choose a reason for hiding this comment

Uh oh!

igorbernstein2 commented Jun 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

codecov bot commented May 31, 2019 •

edited

Loading