Data settings addition #1973

rahulKQL · 2018-10-25T12:54:14Z

This commits contains:

A class with static method for conversion of BigtableOptions to BigtableDataSettings.
Basic test cases for the same, more to add once finalised.

Note: This work is still in progress, opening PR for review purpose.

rahulKQL · 2018-10-25T14:30:46Z

@igorbernstein2 Please review

...igtable-hbase/src/main/java/com/google/cloud/bigtable/hbase/BigtableDataSettingsFactory.java

...ble-hbase/src/test/java/com/google/cloud/bigtable/hbase/TestBigtableDataSettingsFactory.java

igorbernstein2

Looks like a good start. I added some comments. Please add inline TODOs for all settings that are currently unused

...igtable-hbase/src/main/java/com/google/cloud/bigtable/hbase/BigtableDataSettingsFactory.java

rahulKQL · 2018-10-31T11:41:17Z

In this commit:

individual settings method, with retry check.
default method to set RetrySettings.
added/fixed code based on suggestions.
added many inline comment on doubtful points.
Added default RPC timeout as 100ms in case of No Retry

Would add test cases & removed unneeded comments once it gets reviewed.
@igorbernstein2 Could you please have a look at it

...igtable-hbase/src/main/java/com/google/cloud/bigtable/hbase/BigtableDataSettingsFactory.java

igorbernstein2 · 2018-10-31T14:59:07Z

...igtable-hbase/src/main/java/com/google/cloud/bigtable/hbase/BigtableDataSettingsFactory.java

+          .setRetrySettings(defaultRetrySettings(options));
+    } else {
+      builder.readRowsSettings()
+          .setIdleTimeout(ofMillis(options.getRetryOptions().getReadPartialRowTimeoutMillis()))


This is incorrect idleTimeout is the delay for the user leaving a stream open not the timeout for the server. You want to set the streamingWaitTimeout. There are 2 ways to do it:

the rpcTimeout of the RetrySettings gets translated into a waitTimeout

set it in calloptions

1 will work for when retries are enabled. However when retries are disabled, you would have to augment the calloptions at the callsite, which is awkward. Please open a ticket in gax-java to expose waitTimeout at the callable level

I would open an issue stating this above scenario. Thanks

...igtable-hbase/src/main/java/com/google/cloud/bigtable/hbase/BigtableDataSettingsFactory.java

rahulKQL · 2018-11-15T15:26:08Z

@igorbernstein2 Could you please have a look at it.

I will add more junit for it tomorrow.

googlebot · 2018-12-26T11:16:54Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

googlebot · 2018-12-26T11:33:45Z

CLAs look good, thanks!

rahulKQL · 2018-12-26T13:07:58Z

Changes

BigtableVaneerSettingsFactory

Added UserAgent through HeaderProvider with GrpcUtil.USER_AGENT_KEY which internally translated to ManagedChannel here.
Moved to InstantiatingGrpcChannelProvider for ChannelProvider, which gives option for Headers along with channel PoolSize.
Overriding non-idempotent RPCs only when SHORT_RPC_TIMEOUT is other than default value(i.e. 60seconds).
For BigtableTableAdminSettings: overriding Endpoint, CredentialProvider & TransportChannelProvider configuration only.

TestBigtableVaneerSettingsFactory

Added test case which tests BigtableDataClient as well BigtableTableAdminClient with an actual Bigtable, This test case runs only when it founds "test.client.project.id" & "test.client.project.id" VM arguments.

Question:

My purpose for using InstantiatingGrpcChannelProvider is to be able to create channelPools, however
according to this comment, it seems ManagedChannel#WithUserAgnet() should be used to setting user-agent, So for considering multipleChannel:

Should we use the BigtableSession#createChannelConfig as it takes care of creating ChannelPool?

rahulKQL · 2019-01-02T15:08:36Z

@sduskis, @igorbernstein2

Please have a look at this and let me know of your thought.

...lient-core/src/main/java/com/google/cloud/bigtable/config/BigtableVaneerSettingsFactory.java

igorbernstein2 · 2019-01-08T16:08:14Z

...lient-core/src/main/java/com/google/cloud/bigtable/config/BigtableVaneerSettingsFactory.java

+   * @param options a {@link BigtableOptions} object.
+   */
+  private static void buildMutateRowSettings(Builder builder, BigtableOptions options) {
+    builder.mutateRowSettings()


where are the retry codes configured? (here and everywhere else)

As of now, not overriding RetryCodes of BigtableDataSettings.
Defaults retrycode in GCJ:
[DEADLINE_EXCEEDED, UNAVAILABLE] --> sampleRowKeysSettings, readRowsSettings, mutateRowSettings.
empty for bulkMutationsSettings, readModifyWriteRowSettings, checkAndMutateRowSettings.

Default retrycodes in bigtable-client.

However, I can override these with operation specific retry codes.

There are few more defaults for gcj in the overlay. The gapic generated client settings can't express all of the settings, so they are overlaid here:
https://github.com/googleapis/google-cloud-java/blob/master/google-cloud-clients/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/data/v2/stub/EnhancedBigtableStubSettings.java#L273-L321

This is where bulkMutations have their defaults set.

I'm trying to understand why we shouldn't copy the retry codes from cbt?

I think for Non-idempotent operation we can utilize GCJ's default values, as we are overriding it with shortRpcTimeout & no retryCodes (here both adapters have the default value of 1 min).

In case of Idempotent operation default values varies alot &

CBT has retryCode as {DEADLINE_EXCEEDED, UNAVAILABLE, ABORTED, UNAUTHENTICATED}

GCJ's retryCodes are only {DEADLINE_EXCEEDED, UNAVAILABLE}.

...lient-core/src/main/java/com/google/cloud/bigtable/config/BigtableVaneerSettingsFactory.java

igorbernstein2 · 2019-01-09T17:47:31Z

...lient-core/src/main/java/com/google/cloud/bigtable/config/BigtableVaneerSettingsFactory.java

+    adminBuilder.stubSettings()
+        .setCredentialsProvider(buildCredentialProvider(options.getCredentialOptions()));
+
+    String userAgent = BigtableVersionInfo.CORE_USER_AGENT + "," + options.getUserAgent();


Please add something here that would allow us to disambiguate when the hbase client is using the gcj adapter

Does "VENEER_ADAPTER" sound good?

rahulKQL · 2019-01-25T12:56:20Z

Summary of Client Settings changes

After many Iteration now in this PR, as of now we are:

Overriding both client setting's endpoint with BigtableOptions endpoint here.
Utilizing existing CredentialFactory#getCredentials for CredentialProvider.
Providing "VENEER_ADAPTER" in the User-Agent to be able to distinguish GCJ adapters.
Created TransportChannelProvider using InstantiatingGrpcChannelProvider here:
- Kept ChannelsPerCpu to 2 (Though it BigtableOptions has 4 ChannelsPerCpu as default).
- Kept MaxInboundMessageSize to CBT's default(i.e. 256MB).
- For plainText Negotiation using ChannelConfigurator (would this be ok?, as its not stable yet?).
Created buildIdempotentRetrySettings to configure generic RetrySettings.

createBigtableDataSettings
- As of now we are not supporting disabling the retries.
- In case of bulkMutationsSettings, We override:
  - BatchingSettings with BulkOptions values.
  - RetrySettings with idempotentRetrySettings.
  - RetryCodes with BigtableOptions's Retries Code.
- In case of sampleRowKeysSettings, mutateRowSettings and readRowSettings, We overrides
  - RetrySettings with idempotentRetrySettings.
  - RetryCodes with BigtableOptions's Retries Code.
- In case of readRowsSettings, We override
  - RetrySettings with idempotentRetrySettings considering streaming method according to this suggestion.
  - RetryCodes with BigtableOptions's Retries Code.
- In case of checkAndMutateRowSettings & readModifyWriteRowSettings, We override SimpleTimeoutNoRetries when shortRpcTimeoutMs is not default.
createTableAdminSettings
- Only Overrided endpoint, CredentialsProvider & TransportChannelProvider. Do we need to override any specific settings for this?

rahulKQL · 2019-02-01T18:27:29Z

Please have a look & Please let me know if we need more modify any configurations.

...lient-core/src/main/java/com/google/cloud/bigtable/config/BigtableVeneerSettingsFactory.java

igorbernstein2 · 2019-02-11T15:00:25Z

...lient-core/src/main/java/com/google/cloud/bigtable/config/BigtableVeneerSettingsFactory.java

+    //ChannelBuilder with provided value.
+    HeaderProvider headers = FixedHeaderProvider.create(USER_AGENT_KEY.name(), userAgent);
+
+    InstantiatingGrpcChannelProvider.Builder builder =


Please a TODO to refactor google-cloud-java to expose a static defaultTransportChannelProvider method and then customize it.

...lient-core/src/main/java/com/google/cloud/bigtable/config/BigtableVeneerSettingsFactory.java

igorbernstein2 · 2019-02-11T15:45:01Z

...lient-core/src/main/java/com/google/cloud/bigtable/config/BigtableVeneerSettingsFactory.java

+   * than 60_000 ms.
+   */
+  private static void buildReadModifyWriteSettings(Builder builder, long rpcTimeoutMs) {
+    if(rpcTimeoutMs != SHORT_TIMEOUT_MS_DEFAULT) {


why if? (here and above)

If shortTimeOutMs is at default then assuming the client did not customize BigtableOption's settings,
that is why we are not overriding the underlying settings here(or directly using defaults of GCJ's settings).

I think you should override with the BigtableOptions in case the diverge

override with the BigtableOptions in case the diverge

By saying above comment,
do you mean we should check all configuration against their default value(present in BigtableOption) & then override only those which are different?
OR
Should we override only in case of specific operation(say non-streming operations) by checking BigtableOption's present value against BigtableDataSetting's default value?

I just meant you should remove the if statement and always setSimpleTimeoutNoRetries disrarding if the if its the default value or not. If we decide that to change the gapic config to 30 secs, but forget to change SHORT_TIMEOUT_MS_DEFAULT we will have confusing behavior: the user will check their BigtableOptions and see SHORT_TIMEOUT_MS_DEFAULT = 1 minute, but their rpcs will exceed deadline @ 30secs

Thanks for the clarification!!
I followed the same here.

igorbernstein2 · 2019-02-17T17:13:03Z

I just submitted googleapis/google-cloud-java#4507, it should help with the transport builder

igorbernstein2

LGTM, except for a couple of nits

...lient-core/src/main/java/com/google/cloud/bigtable/config/BigtableVeneerSettingsFactory.java

igorbernstein2 · 2019-02-19T20:12:49Z

...lient-core/src/main/java/com/google/cloud/bigtable/config/BigtableVeneerSettingsFactory.java

+    RetrySettings.Builder retryBuilder = retrySettings.toBuilder();
+
+    if (retryOptions.allowRetriesWithoutTimestamp()) {
+      LOG.warn("Retries without Timestamp does not support yet.");


This should throw instead of warn

rahulKQL · 2019-02-28T13:09:37Z

@sduskis

Will be just rebasing and updating these three last comments

only adding fromBigtableOptions static method adding separate methods for each settings Added check for retry & remvoed unneeded setter

renamed Factory methods Reverted BulkMutationSettings changes refactored Credentail Provider

Used constant for maxRetryDelay & explicitly set the maxAttempts to zero

- Now using EnhancedBigtableDataSettings.defaultGrpcChannelPRovider for channel provider configuration. - Updated existed BigtableDataSetttings.Builder to EnhancedBigtableDataSettings.Builder. - changed user agent for vaneer adapter to lowercase. - throwing exception in case of allowRetriesWithoutTimestamp.

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 25, 2018

rahulKQL commented Oct 25, 2018

View reviewed changes

...igtable-hbase/src/main/java/com/google/cloud/bigtable/hbase/BigtableDataSettingsFactory.java Outdated Show resolved Hide resolved

sduskis suggested changes Oct 29, 2018

View reviewed changes

...ble-hbase/src/test/java/com/google/cloud/bigtable/hbase/TestBigtableDataSettingsFactory.java Outdated Show resolved Hide resolved

sduskis requested a review from igorbernstein2 October 29, 2018 16:24

igorbernstein2 reviewed Oct 30, 2018

View reviewed changes

...igtable-hbase/src/main/java/com/google/cloud/bigtable/hbase/BigtableDataSettingsFactory.java Outdated Show resolved Hide resolved

igorbernstein2 reviewed Oct 31, 2018

View reviewed changes

rahulKQL commented Nov 1, 2018

View reviewed changes

...igtable-hbase/src/main/java/com/google/cloud/bigtable/hbase/BigtableDataSettingsFactory.java Outdated Show resolved Hide resolved

igorbernstein2 reviewed Nov 1, 2018

View reviewed changes

rahulKQL mentioned this pull request Nov 14, 2018

fixed method names changes rahulKQL/java-bigtable-hbase#3

Merged

googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Dec 26, 2018

rahulKQL changed the base branch from master to bigtable-client-build-latest December 26, 2018 11:20

rahulKQL changed the base branch from bigtable-client-build-latest to master December 26, 2018 11:20

googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Dec 26, 2018

rahulKQL mentioned this pull request Jan 7, 2019

Admin interface as GAX admin client #2040

Merged

igorbernstein2 requested changes Jan 8, 2019

View reviewed changes

igorbernstein2 reviewed Jan 9, 2019

View reviewed changes

rahulKQL force-pushed the dataSettings_addition branch 2 times, most recently from bc64044 to bffb90f Compare January 25, 2019 03:11

rahulKQL mentioned this pull request Feb 1, 2019

WIP: Added ModelRowAdapter for models.Row to Result #2056

Closed

igorbernstein2 requested changes Feb 11, 2019

View reviewed changes

rahulKQL mentioned this pull request Feb 14, 2019

[Bigtable] Added HeaderProvider onto ClientContext#create settings googleapis/google-cloud-java#4491

Merged

igorbernstein2 approved these changes Feb 19, 2019

View reviewed changes

sduskis approved these changes Feb 28, 2019

View reviewed changes

rahulKQL added 9 commits February 28, 2019 18:41

static method for BigtableDataSettings creation

a31bf6d

only adding fromBigtableOptions static method adding separate methods for each settings Added check for retry & remvoed unneeded setter

Adding BigtableDataClient & BigtableAdminClient

5e32f95

renamed Factory methods Reverted BulkMutationSettings changes refactored Credentail Provider

Added User-Agent test along with more feedback changes

ffe95f8

added ChannelPerCPU config

60c30c5

Adding Refactored BigtableVeneerSettings

d5d0594

Rebase & Refactored retryStatusCodes

03f805a

Addressing Feedback for BigtableVeneerSettings

14c80af

Update BigtableVeneerSettingsFactory.java

3b219d7

Used constant for maxRetryDelay & explicitly set the maxAttempts to zero

rahulKQL force-pushed the dataSettings_addition branch from f918830 to d98cd29 Compare February 28, 2019 13:18

sduskis merged commit 0d751ae into googleapis:master Feb 28, 2019

rahulKQL deleted the dataSettings_addition branch April 4, 2019 08:08

Data settings addition #1973

Data settings addition #1973

Uh oh!

Conversation

rahulKQL commented Oct 25, 2018

Uh oh!

rahulKQL commented Oct 25, 2018

Uh oh!

Uh oh!

Uh oh!

igorbernstein2 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rahulKQL commented Oct 31, 2018

In this commit:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rahulKQL commented Nov 15, 2018

Uh oh!

googlebot commented Dec 26, 2018

Uh oh!

googlebot commented Dec 26, 2018

Uh oh!

rahulKQL commented Dec 26, 2018

Changes

BigtableVaneerSettingsFactory

TestBigtableVaneerSettingsFactory

Question:

Uh oh!

rahulKQL commented Jan 2, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rahulKQL commented Jan 25, 2019

Summary of Client Settings changes

Uh oh!

rahulKQL commented Feb 1, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rahulKQL Feb 12, 2019 •

edited

Loading