Skip to content

fixed method names changes #3

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
rahulKQL merged 4 commits into from
Nov 14, 2018
Merged

fixed method names changes #3

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
152a42f
fixed method names changes
rahulKQL Nov 6, 2018
46e0ac3
refactored code based on feedback
rahulKQL Nov 13, 2018
49def1e
added logger & precondition
rahulKQL Nov 14, 2018
407a0e3
fixed names & error messages
rahulKQL Nov 14, 2018
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
94 changes: 51 additions & 43 deletions ...able-hbase/src/main/java/com/google/cloud/bigtable/hbase/BigtableDataSettingsFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/
package com.google.cloud.bigtable.hbase;

import static com.google.api.client.util.Preconditions.checkState;
import static org.threeten.bp.Duration.ofMillis;

import java.io.FileInputStream;
Expand All @@ -23,16 +24,17 @@
import java.security.GeneralSecurityException;

import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.hbase.client.Mutation;
import org.threeten.bp.Duration;

import com.google.api.client.util.Preconditions;
import com.google.api.gax.batching.BatchingSettings;
import com.google.api.gax.batching.FlowControlSettings;
import com.google.api.gax.core.CredentialsProvider;
import com.google.api.gax.core.FixedCredentialsProvider;
import com.google.api.gax.core.NoCredentialsProvider;
import com.google.api.gax.grpc.GrpcTransportChannel;
import com.google.api.gax.retrying.RetrySettings;
import com.google.api.gax.rpc.FixedHeaderProvider;
import com.google.api.gax.rpc.FixedTransportChannelProvider;
import com.google.cloud.bigtable.config.BigtableOptions;
import com.google.cloud.bigtable.config.BulkOptions;
Expand All @@ -46,7 +48,6 @@
import com.google.cloud.bigtable.data.v2.stub.BigtableStubSettings;

import io.grpc.ManagedChannelBuilder;
import io.grpc.internal.GrpcUtil;

/**
* Static methods to convert an instance of {@link Configuration} or {@link BigtableOptions} to a
Expand All @@ -65,49 +66,40 @@ public class BigtableDataSettingsFactory {
*/
public static BigtableDataSettings fromBigtableOptions(final BigtableOptions options)
throws IOException, GeneralSecurityException {
if (!options.getRetryOptions().enableRetries()) {
throw new IllegalStateException(
"Retry is must for BigtableDataSettings configuration from BigtableOptions.");
}

checkState(!options.getRetryOptions().enableRetries(), "Disabling retries is not currently supported.");

BigtableDataSettings.Builder builder = BigtableDataSettings.newBuilder();

InstanceName instanceName = InstanceName.newBuilder().setProject(options.getProjectId())
.setInstance(options.getInstanceId()).build();
builder.setInstanceName(instanceName);
builder.setAppProfileId(options.getAppProfileId());

LOG.debug("endpoint host %s.", options.getDataHost());
LOG.debug("endpoint host %s.", options.getPort());
builder.setEndpoint(options.getDataHost() + ":" + options.getPort());

buildCredentialProvider(builder, options.getCredentialOptions());

buildBulkOptions(builder, options);
buildCredentialProviderSettings(builder, options.getCredentialOptions());

buildCheckAndMutateRow(builder, options.getCallOptionsConfig().getShortRpcTimeoutMs());
buildBulkMutationsSettings(builder, options);

buildReadModifyWrite(builder, options.getCallOptionsConfig().getShortRpcTimeoutMs());
buildCheckAndMutateRowSettings(builder, options.getCallOptionsConfig().getShortRpcTimeoutMs());

buildReadRows(builder, options);
buildReadModifyWriteSettings(builder, options.getCallOptionsConfig().getShortRpcTimeoutMs());

buildMutateRow(builder, options);
buildReadRowsSettings(builder, options);

buildSampleRowKeys(builder, options);
buildMutateRowSettings(builder, options);

// TODO: would it map to GrpcHeaderInterceptor? or we should build userAgent
// using ManagedChannelBuilder AND
builder.setHeaderProvider(
FixedHeaderProvider.create(GrpcUtil.USER_AGENT_KEY.name(), options.getUserAgent()));
buildSampleRowKeysSettings(builder, options);

// TODO: implementation for channelCount or channelPerCPU
ManagedChannelBuilder channelBuilder = ManagedChannelBuilder
Copy link
Owner Author

@rahulKQL rahulKQL Nov 6, 2018

Choose a reason for hiding this comment

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

Related to UserAgent comment

I have verified using debug mode, User Agent is being preserved in TransportChannelProvider. So that is the way to confirm, right?

Copy link
Owner Author

@rahulKQL rahulKQL Nov 12, 2018

Choose a reason for hiding this comment

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

Having userAgenet value preserved in TransportChannel is good.

.forAddress(options.getDataHost(), options.getPort())//
.forAddress(options.getDataHost(), options.getPort())
.userAgent(options.getUserAgent());

if (options.usePlaintextNegotiation()) {
channelBuilder.usePlaintext();
}

builder.setTransportChannelProvider(
FixedTransportChannelProvider.create(GrpcTransportChannel.create(channelBuilder.build())));

Expand All @@ -120,23 +112,22 @@ public static BigtableDataSettings fromBigtableOptions(final BigtableOptions opt
* @param builder a {@link BigtableDataSettings.Builder} object.
* @param options a {@link BigtableOptions} object.
*/
private static void buildBulkOptions(Builder builder, BigtableOptions options) {
private static void buildBulkMutationsSettings(Builder builder, BigtableOptions options) {
BulkOptions bulkOptions = options.getBulkOptions();
BatchingSettings.Builder batchSettingsBuilder = BatchingSettings.newBuilder();

FlowControlSettings.Builder flowControlBuilder =
FlowControlSettings.newBuilder()
.setMaxOutstandingRequestBytes(bulkOptions.getMaxMemory());

long autoFlushMs = bulkOptions.getAutoflushMs();
long bulkMaxRowKeyCount = bulkOptions.getBulkMaxRowKeyCount();
long maxInflightRpcs = bulkOptions.getMaxInflightRpcs();

if (autoFlushMs > 0) {
batchSettingsBuilder.setDelayThreshold(Duration.ofMillis(autoFlushMs));
}
FlowControlSettings.Builder flowControlBuilder = FlowControlSettings.newBuilder();
if (maxInflightRpcs > 0) {
flowControlBuilder.setMaxOutstandingElementCount(maxInflightRpcs * bulkMaxRowKeyCount);
flowControlBuilder
.setMaxOutstandingRequestBytes(bulkOptions.getMaxMemory())
.setMaxOutstandingElementCount(maxInflightRpcs * bulkMaxRowKeyCount);
}

batchSettingsBuilder
Expand All @@ -156,9 +147,9 @@ private static void buildBulkOptions(Builder builder, BigtableOptions options) {
* @param builder a {@link BigtableDataSettings.Builder} object.
* @param bulkMutation a {@link BulkOptions} object.
*/
private static void buildSampleRowKeys(Builder builder, BigtableOptions options) {
private static void buildSampleRowKeysSettings(Builder builder, BigtableOptions options) {
builder.sampleRowKeysSettings()
.setRetrySettings(defaultRetrySettings(options));
.setRetrySettings(buildIdempotentRetrySettings(options));
}

/**
Expand All @@ -167,9 +158,9 @@ private static void buildSampleRowKeys(Builder builder, BigtableOptions options)
* @param builder a {@link BigtableDataSettings.Builder} object.
* @param bulkMutation a {@link BulkOptions} object.
*/
private static void buildMutateRow(Builder builder, BigtableOptions options) {
private static void buildMutateRowSettings(Builder builder, BigtableOptions options) {
builder.mutateRowSettings()
.setRetrySettings(defaultRetrySettings(options));
.setRetrySettings(buildIdempotentRetrySettings(options));
}

/**
Expand All @@ -178,10 +169,24 @@ private static void buildMutateRow(Builder builder, BigtableOptions options) {
* @param builder a {@link BigtableDataSettings.Builder} object.
* @param bulkMutation a {@link BulkOptions} object.
*/
private static void buildReadRows(Builder builder, BigtableOptions options) {
// TODO: set readPartialRowTimeout for watchdog timer, taken BigtableSession#setupWatchdog()
private static void buildReadRowsSettings(Builder builder, BigtableOptions options) {
RetryOptions retryOptions = options.getRetryOptions();

RetrySettings.Builder retryBuilder = RetrySettings.newBuilder()
.setInitialRetryDelay(ofMillis(retryOptions.getInitialBackoffMillis()))
.setRetryDelayMultiplier(retryOptions.getBackoffMultiplier())
.setMaxRetryDelay(ofMillis(retryOptions.getMaxElapsedBackoffMillis()))
.setMaxAttempts(retryOptions.getMaxScanTimeoutRetries());

// configurations for RPC timeouts
Duration readPartialRowTimeout = ofMillis(retryOptions.getReadPartialRowTimeoutMillis());
retryBuilder
.setInitialRpcTimeout(readPartialRowTimeout)
.setMaxRpcTimeout(readPartialRowTimeout)
.setTotalTimeout(ofMillis(options.getCallOptionsConfig().getLongRpcTimeoutMs()));

Copy link
Owner Author

@rahulKQL rahulKQL Nov 6, 2018

Choose a reason for hiding this comment

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

Related to readPatialRowsTimeout

After reading his comment, My approach here is to set RetrySettings separately for ReadRows & use buildIdempotentRetrySettings for sampleRowKeys & mutateRow (Assuming only these two are non streaming methods).

builder.readRowsSettings()
.setRetrySettings(defaultRetrySettings(options));
.setRetrySettings(retryBuilder.build());
}

/**
Expand All @@ -190,7 +195,7 @@ private static void buildReadRows(Builder builder, BigtableOptions options) {
* @param builder a {@link BigtableDataSettings.Builder} object.
* @param bulkMutation a {@link BulkOptions} object.
*/
private static void buildReadModifyWrite(Builder builder, long rpcTimeoutMs) {
private static void buildReadModifyWriteSettings(Builder builder, long rpcTimeoutMs) {
builder.readModifyWriteRowSettings()
.setSimpleTimeoutNoRetries(ofMillis(rpcTimeoutMs));
}
Expand All @@ -201,7 +206,7 @@ private static void buildReadModifyWrite(Builder builder, long rpcTimeoutMs) {
* @param builder a {@link BigtableDataSettings.Builder} object.
* @param bulkMutation a {@link BulkOptions} object.
*/
private static void buildCheckAndMutateRow(Builder builder, long rpcTimeoutMs) {
private static void buildCheckAndMutateRowSettings(Builder builder, long rpcTimeoutMs) {
builder.checkAndMutateRowSettings()
.setSimpleTimeoutNoRetries(ofMillis(rpcTimeoutMs));
}
Expand All @@ -212,7 +217,7 @@ private static void buildCheckAndMutateRow(Builder builder, long rpcTimeoutMs) {
* @param builder a {@link BigtableDataSettings.Builder} object.
* @param bulkMutation a {@link BulkOptions} object.
*/
private static RetrySettings defaultRetrySettings(BigtableOptions options) {
private static RetrySettings buildIdempotentRetrySettings(BigtableOptions options) {
RetryOptions retryOptions = options.getRetryOptions();

RetrySettings.Builder retryBuilder = RetrySettings.newBuilder()
Expand All @@ -222,12 +227,15 @@ private static RetrySettings defaultRetrySettings(BigtableOptions options) {
.setMaxAttempts(retryOptions.getMaxScanTimeoutRetries());

// configurations for RPC timeouts
Duration shortRpcTimeout = ofMillis(options.getCallOptionsConfig().getShortRpcTimeoutMs());
retryBuilder
.setInitialRpcTimeout(ofMillis(options.getCallOptionsConfig().getShortRpcTimeoutMs()))
.setMaxRpcTimeout(ofMillis(retryOptions.getReadPartialRowTimeoutMillis()))
.setInitialRpcTimeout(shortRpcTimeout)
.setMaxRpcTimeout(shortRpcTimeout)
.setTotalTimeout(ofMillis(options.getCallOptionsConfig().getLongRpcTimeoutMs()));

// TODO: an option to set RetryOptions#allowRetriesWithoutTimestamp

if (retryOptions.allowRetriesWithoutTimestamp()) {
LOG.warn("Retries without Timestamp doesn't support, please use Mutation#createUnsafe");
Copy link
Owner Author

@rahulKQL rahulKQL Nov 14, 2018

Choose a reason for hiding this comment

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

@ajaaym I remember Solomon told about hbase's Mutation but I didn't any unsafe mutation in hbase's Mutation.. Could you please tell what method should we log.. or "just not support" is enough?

}
return retryBuilder.build();
}

Expand All @@ -239,7 +247,7 @@ private static RetrySettings defaultRetrySettings(BigtableOptions options) {
* @throws FileNotFoundException
* @throws IOException
*/
private static void buildCredentialProvider(Builder builder, CredentialOptions credentialOptions)
private static void buildCredentialProviderSettings(Builder builder, CredentialOptions credentialOptions)
throws FileNotFoundException, IOException {
CredentialsProvider credentialsProvider = NoCredentialsProvider.create();

Expand Down
7 changes: 5 additions & 2 deletions ...-hbase/src/test/java/com/google/cloud/bigtable/hbase/TestBigtableDataSettingsFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,11 +58,14 @@ public void testInstanceIdIsRequired() throws IOException, GeneralSecurityExcept
BigtableDataSettingsFactory.fromBigtableOptions(options);
}

@Test(expected = NullPointerException.class)
@Test
public void testWithoutUserAgent() throws IOException, GeneralSecurityException {
BigtableOptions options = BigtableOptions.builder().setProjectId(TEST_PROJECT_ID)
.setInstanceId(TEST_INSTANCE_ID).build();
BigtableDataSettingsFactory.fromBigtableOptions(options);
BigtableDataSettings dataSettings = BigtableDataSettingsFactory.fromBigtableOptions(options);
// TODO: Need to assert UserAgent to null & add more test cases
Assert.assertEquals(TEST_PROJECT_ID, dataSettings.getInstanceName().getProject());
Assert.assertEquals(TEST_INSTANCE_ID, dataSettings.getInstanceName().getInstance());
}

@Test
Expand Down