Skip to content

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

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

Merged
igorbernstein2 merged 5 commits into from
Jun 19, 2019
Merged

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

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
00a69c7
Documented defaults timeout values in the Bigtable Settings
rahulKQL May 7, 2019
bdd4421
javadoc fix
rahulKQL May 8, 2019
6ca6923
Addressing feedback comments
rahulKQL May 31, 2019
d62e034
Updated Javadoc according to feedback
rahulKQL Jun 13, 2019
c467bea
addressing feedback comments
rahulKQL Jun 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
135 changes: 129 additions & 6 deletions ...le/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubSettings.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,13 @@
* <li>The default service address (bigtable.googleapis.com) and default port (443) are used.
* <li>Credentials are acquired automatically through Application Default Credentials.
* <li>Retries are configured for idempotent methods but not for non-idempotent methods.
* <li>This settings are mixed for streaming and non-streaming operation. In case of streaming
* operations, RPC timeout applies to each row and for the non-streaming operations, RPC
* timeout considered as deadline for every call.
* <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.
Copy link
Copy Markdown
Contributor

@igorbernstein2 igorbernstein2 Jun 11, 2019

Choose a reason for hiding this comment

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

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)

Copy link
Copy Markdown
Contributor Author

@rahulKQL rahulKQL Jun 13, 2019

Choose a reason for hiding this comment

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

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.

* </ul>
*
* <p>The only required setting is the instance name.
Expand Down Expand Up @@ -185,22 +192,92 @@ public static GoogleCredentialsProvider.Builder defaultCredentialsProviderBuilde
.setJwtEnabledScopes(JWT_ENABLED_SCOPES);
}

/** Returns the object with the settings used for calls to ReadRows. */
/**
* Returns the object with the settings used for calls to ReadRows.
*
* <p>This is idempotent and streaming operation.
*
* <p>The idle timeout is configured via {@code ServerStreamingCallSettings#getIdleTimeout}. This
Copy link
Copy Markdown
Contributor

@igorbernstein2 igorbernstein2 Jun 11, 2019

Choose a reason for hiding this comment

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

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

* timer will terminate any stream that has not has seen any demand or activity in the configured
* interval. To turn off idle checks, set the idleTimeout to {@link Duration#ZERO}.
Comment thread
rahulKQL marked this conversation as resolved.
Outdated
*
* <p>Retries will be attempted if RPC failed with {@link Code#DEADLINE_EXCEEDED} and {@link
* 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.
Copy link
Copy Markdown
Contributor

@igorbernstein2 igorbernstein2 Jun 11, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@rahulKQL rahulKQL Jun 13, 2019

Choose a reason for hiding this comment

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

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

*
* <p>RPC timeout is 20 seconds.
Copy link
Copy Markdown
Contributor

@igorbernstein2 igorbernstein2 Jun 11, 2019

Choose a reason for hiding this comment

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

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.

*
* <p>Total timeout for this operation is 60 mins or 1 hour.
*
* @see RetrySettings for more explanation.
*/
public ServerStreamingCallSettings<Query, Row> readRowsSettings() {
return readRowsSettings;
}

/** Returns the object with the settings used for calls to SampleRowKeys. */
/**
* Returns the object with the settings used for calls to SampleRowKeys.
*
* <p>This is idempotent and non-streaming operation.
*
* <p>Retries will be attempted if RPC failed with {@link Code#DEADLINE_EXCEEDED} and {@link
* 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.
*
* <p>RPC timeout is 20 seconds.
*
* <p>Total timeout for this operation is 600 seconds or 10 mins.
Copy link
Copy Markdown
Contributor

@igorbernstein2 igorbernstein2 Jun 11, 2019

Choose a reason for hiding this comment

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

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.

*
* @see RetrySettings for more explanation.
*/
public UnaryCallSettings<String, List<KeyOffset>> sampleRowKeysSettings() {
return sampleRowKeysSettings;
}

/** Returns the object with the settings used for point reads via ReadRows. */
/**
* Returns the object with the settings used for point reads via ReadRows.
*
* <p>This is an idempotent and non-streaming operation.
*
* <p>
*
* <p>Retries will be attempted if RPC failed with {@link Code#DEADLINE_EXCEEDED} and {@link
* 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.
*
* <p>RPC timeout is 20 seconds.
*
* <p>Total timeout for this operation is 600 seconds or 10 mins.
*
* @see RetrySettings for more explanation.
*/
public UnaryCallSettings<Query, Row> readRowSettings() {
return readRowSettings;
}

/** Returns the object with the settings used for calls to MutateRow. */
/**
* Returns the object with the settings used for calls to MutateRow.
*
* <p>This is an idempotent and non-streaming operation.
*
* <p>Retries will be attempted if RPC failed * with {@link Code#DEADLINE_EXCEEDED} and {@link
* 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.
*
* <p>RPC timeout is 20 seconds.
*
* <p>Total timeout for this operation is 600 seconds or 10 mins.
*
* @see RetrySettings for more explanation.
*/
public UnaryCallSettings<RowMutation, Void> mutateRowSettings() {
return mutateRowSettings;
}
Expand All @@ -211,17 +288,63 @@ public UnaryCallSettings<RowMutation, Void> mutateRowSettings() {
* <p>Please note that these settings will affect both manually batched calls
* (bulkMutateRowsCallable) and automatic batched calls (bulkMutateRowsBatchingCallable). The
* {@link RowMutation} request signature is ignored for the manual batched calls.
*
* <p>Retries will be attempted if RPC are failed with {@link Code#DEADLINE_EXCEEDED}, {@link
* Code#UNAVAILABLE} or {@link Code#ABORTED} 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.
*
* <p>RPC timeout is 20 seconds.
*
* <p>Total timeout for this operation is 600 seconds or 10 mins.
*
* <p>On breach of certain triggers, this batch initiates the processing of accumulated requests.
* These triggers could be configured via {@link BatchingSettings}. Currently it triggers when:
*
* <ul>
* <li>100 or more requests are sent.
* <li>Requests size reaches to 20MB.
* <li>An interval of 1 second passes after batching initialization or last processed batch.
* </ul>
*
* <p>When size of the request in processing state reaches default value of 100MB or the request
* count reaches default count of 1000 then the batching will be blocked till some of the request
* resolves.
*
* @see RetrySettings for more explanation.
* @see BatchingSettings for batch related configuration explanation.
*/
public BatchingCallSettings<RowMutation, Void> bulkMutateRowsSettings() {
return bulkMutateRowsSettings;
}

/** Returns the object with the settings used for calls to CheckAndMutateRow. */
/**
* Returns the object with the settings used for calls to CheckAndMutateRow.
*
* <p>This is a non-idempotent and non-streaming operation.
*
* <p>By default non-idempotent operations would not reattempt in case of RPC failure.
*
* <p>Total timeout for this operation is 600 seconds or 10 mins.
*
* @see RetrySettings for more explanation.
*/
public UnaryCallSettings<ConditionalRowMutation, Boolean> checkAndMutateRowSettings() {
return checkAndMutateRowSettings;
}

/** Returns the object with the settings used for calls to ReadModifyWriteRow. */
/**
* Returns the object with the settings used for calls to ReadModifyWriteRow.
*
* <p>This is a non-idempotent and non-streaming operation.
*
* <p>By default non-idempotent operations would not reattempt in case of RPC failure.
*
* <p>Total timeout for this operation is 600 seconds or 10 mins.
*
* @see RetrySettings for more explanation.
*/
public UnaryCallSettings<ReadModifyWriteRow, Row> readModifyWriteRowSettings() {
return readModifyWriteRowSettings;
}
Expand Down