Skip to content
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.

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

feat: introduce java.time methods and variables #2826

Merged
merged 3 commits into from
Dec 2, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -189,8 +189,7 @@ final class GrpcStorageImpl extends BaseService<StorageOptions>
public void close() throws Exception {
try (StorageClient s = storageClient) {
s.shutdownNow();
org.threeten.bp.Duration terminationAwaitDuration =
getOptions().getTerminationAwaitDuration();
java.time.Duration terminationAwaitDuration = getOptions().getTerminationAwaitDuration();
s.awaitTermination(terminationAwaitDuration.toMillis(), TimeUnit.MILLISECONDS);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,15 @@

package com.google.cloud.storage;

import static com.google.api.gax.util.TimeConversionUtils.toJavaTimeDuration;
import static com.google.api.gax.util.TimeConversionUtils.toThreetenDuration;
import static com.google.common.base.Preconditions.checkArgument;
import static java.util.Objects.requireNonNull;

import com.google.api.core.ApiClock;
import com.google.api.core.BetaApi;
import com.google.api.core.InternalApi;
import com.google.api.core.ObsoleteApi;
import com.google.api.gax.core.CredentialsProvider;
import com.google.api.gax.core.FixedCredentialsProvider;
import com.google.api.gax.core.NoCredentialsProvider;
Expand Down Expand Up @@ -97,7 +100,6 @@
import java.util.Objects;
import java.util.Set;
import org.checkerframework.checker.nullness.qual.NonNull;
import org.threeten.bp.Duration;

/** @since 2.14.0 */
@TransportCompatibility(Transport.GRPC)
Expand All @@ -110,7 +112,7 @@ public final class GrpcStorageOptions extends StorageOptions
private static final String DEFAULT_HOST = "https://storage.googleapis.com";

private final GrpcRetryAlgorithmManager retryAlgorithmManager;
private final Duration terminationAwaitDuration;
private final java.time.Duration terminationAwaitDuration;
private final boolean attemptDirectPath;
private final boolean enableGrpcClientMetrics;

Expand All @@ -126,7 +128,8 @@ private GrpcStorageOptions(Builder builder, GrpcStorageDefaults serviceDefaults)
builder.storageRetryStrategy, serviceDefaults.getStorageRetryStrategy()));
this.terminationAwaitDuration =
MoreObjects.firstNonNull(
builder.terminationAwaitDuration, serviceDefaults.getTerminationAwaitDuration());
builder.terminationAwaitDuration,
serviceDefaults.getTerminationAwaitDurationDuration());
this.attemptDirectPath = builder.attemptDirectPath;
this.enableGrpcClientMetrics = builder.enableGrpcClientMetrics;
this.grpcClientMetricsManuallyEnabled = builder.grpcMetricsManuallyEnabled;
Expand All @@ -145,7 +148,7 @@ GrpcRetryAlgorithmManager getRetryAlgorithmManager() {
}

@InternalApi
Duration getTerminationAwaitDuration() {
java.time.Duration getTerminationAwaitDuration() {
return terminationAwaitDuration;
}

Expand Down Expand Up @@ -314,17 +317,17 @@ private Tuple<StorageSettings, Opts<UserProject>> resolveSettingsAndOpts() throw
// seconds.
// To allow read streams to have longer lifespans, crank up their timeouts, instead rely
// on idleTimeout below.
.setLogicalTimeout(Duration.ofDays(28))
.setLogicalTimeout(java.time.Duration.ofDays(28))
.build();
Duration totalTimeout = baseRetrySettings.getTotalTimeout();
java.time.Duration totalTimeout = baseRetrySettings.getTotalTimeoutDuration();
Set<Code> startResumableWriteRetryableCodes =
builder.startResumableWriteSettings().getRetryableCodes();

// retries for unary methods are generally handled at a different level, except
// StartResumableWrite
builder.applyToAllUnaryMethods(
input -> {
input.setSimpleTimeoutNoRetries(totalTimeout);
input.setSimpleTimeoutNoRetriesDuration(totalTimeout);
return null;
});

Expand All @@ -342,7 +345,7 @@ private Tuple<StorageSettings, Opts<UserProject>> resolveSettingsAndOpts() throw
// for reads, the stream can be held open for a long time in order to read all bytes,
// this is totally valid. instead we want to monitor if the stream is doing work and if not
// timeout.
.setIdleTimeout(totalTimeout);
.setIdleTimeoutDuration(totalTimeout);
return Tuple.of(builder.build(), defaultOpts);
}

Expand Down Expand Up @@ -412,7 +415,7 @@ protected boolean shouldRefreshService(Storage cachedService) {
public static final class Builder extends StorageOptions.Builder {

private StorageRetryStrategy storageRetryStrategy;
private Duration terminationAwaitDuration;
private java.time.Duration terminationAwaitDuration;
private boolean attemptDirectPath = GrpcStorageDefaults.INSTANCE.isAttemptDirectPath();
private boolean enableGrpcClientMetrics =
GrpcStorageDefaults.INSTANCE.isEnableGrpcClientMetrics();
Expand All @@ -436,6 +439,15 @@ public static final class Builder extends StorageOptions.Builder {
this.blobWriteSessionConfig = gso.blobWriteSessionConfig;
}

/**
* This method is obsolete. Use {@link #setTerminationAwaitDurationDuration(java.time.Duration)}
* instead.
*/
@ObsoleteApi("Use setTerminationAwaitDurationDuration(java.time.Duration) instead")
public Builder setTerminationAwaitDuration(org.threeten.bp.Duration terminationAwaitDuration) {
return setTerminationAwaitDurationDuration(toJavaTimeDuration(terminationAwaitDuration));
}

/**
* Set the maximum duration in which to await termination of any outstanding requests when
* calling {@link Storage#close()}
Expand All @@ -444,7 +456,8 @@ public static final class Builder extends StorageOptions.Builder {
* @return the builder
* @since 2.14.0
*/
public Builder setTerminationAwaitDuration(Duration terminationAwaitDuration) {
public Builder setTerminationAwaitDurationDuration(
java.time.Duration terminationAwaitDuration) {
this.terminationAwaitDuration =
requireNonNull(terminationAwaitDuration, "terminationAwaitDuration must be non null");
return this;
Expand Down Expand Up @@ -652,9 +665,15 @@ public StorageRetryStrategy getStorageRetryStrategy() {
return StorageRetryStrategy.getDefaultStorageRetryStrategy();
}

/** This method is obsolete. Use {@link #getTerminationAwaitDurationDuration()} instead. */
@ObsoleteApi("Use getTerminationAwaitDurationDuration() instead")
public org.threeten.bp.Duration getTerminationAwaitDuration() {
return toThreetenDuration(getTerminationAwaitDurationDuration());
}

/** @since 2.14.0 */
public Duration getTerminationAwaitDuration() {
return Duration.ofMinutes(1);
public java.time.Duration getTerminationAwaitDurationDuration() {
return java.time.Duration.ofMinutes(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we've seen this case before (or at least I don't recall). But if the method ends with Duration then we come across DurationDuration and I think that's even more confusing.

Maybe we want to keep this for consistency with all other SDK methods, but I'm thinking that naming scheme isn't ideal.

Perhaps getTerminationAwaitJavaTimeDuration()? Open to suggestions for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cross linking googleapis/java-datastore#1671 (comment)

I like JavaTimeDuration as the suffix. Let's use it in datastore too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with the point in googleapis/java-datastore#1671 (comment) saying that using DurationJavaTime keeps the prefixes consistent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to confirm this with @sydney-munro and @JesseLovelace

}

/** @since 2.14.0 */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import com.google.common.base.Strings;
import java.io.IOException;
import java.io.InputStream;
import java.time.Duration;
import java.util.ArrayList;
import java.util.List;
import java.util.UUID;
Expand All @@ -44,7 +45,6 @@
import java.util.concurrent.TimeoutException;
import java.util.logging.Level;
import java.util.logging.Logger;
import org.threeten.bp.Duration;

/**
* Utility to create a remote storage configuration for testing. Storage options can be obtained via
Expand Down Expand Up @@ -233,13 +233,13 @@ public static RemoteStorageHelper create() throws StorageHelperException {
private static RetrySettings retrySettings() {
return RetrySettings.newBuilder()
.setMaxAttempts(10)
.setMaxRetryDelay(Duration.ofMillis(30000L))
.setTotalTimeout(Duration.ofMillis(120000L))
.setInitialRetryDelay(Duration.ofMillis(250L))
.setMaxRetryDelayDuration(Duration.ofMillis(30000L))
.setTotalTimeoutDuration(Duration.ofMillis(120000L))
.setInitialRetryDelayDuration(Duration.ofMillis(250L))
.setRetryDelayMultiplier(1.0)
.setInitialRpcTimeout(Duration.ofMillis(120000L))
.setInitialRpcTimeoutDuration(Duration.ofMillis(120000L))
.setRpcTimeoutMultiplier(1.0)
.setMaxRpcTimeout(Duration.ofMillis(120000L))
.setMaxRpcTimeoutDuration(Duration.ofMillis(120000L))
.build();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,10 @@
import io.grpc.Status.Code;
import io.grpc.stub.StreamObserver;
import java.io.IOException;
import java.time.Duration;
import java.util.Iterator;
import java.util.concurrent.TimeUnit;
import org.junit.Test;
import org.threeten.bp.Duration;

/**
* ReadObject leverages gRPC ServerStream to read a stream of ReadObjectResponse messages spanning
Expand All @@ -48,7 +48,7 @@
* #getTotalTimeout()}, gax will interrupt the stream with a DEADLINE_EXCEEDED error.
*
* <p>Instead of relying on total stream timeout, we rely on idleTimeout for the stream via {@link
* com.google.api.gax.rpc.ServerStreamingCallSettings.Builder#setIdleTimeout(Duration)}.
* com.google.api.gax.rpc.ServerStreamingCallSettings.Builder#setIdleTimeoutDuration(Duration)}.
*
* <p>These tests force specific timeout scenarios to happen against an in-process grpc server to
* ensure our configuration of the StorageClient properly translates to the behavior we want.
Expand Down Expand Up @@ -117,7 +117,7 @@ public void readObject(
.setRetrySettings(
RetrySettings.newBuilder()
.setMaxAttempts(3)
.setTotalTimeout(Duration.ofMillis(totalTimeoutMillis))
.setTotalTimeoutDuration(Duration.ofMillis(totalTimeoutMillis))
.build())
.build()
.getStorageSettings();
Expand Down Expand Up @@ -196,7 +196,7 @@ public void readObject(
.setRetrySettings(
RetrySettings.newBuilder()
.setMaxAttempts(3)
.setTotalTimeout(Duration.ofMillis(totalTimeoutMillis))
.setTotalTimeoutDuration(Duration.ofMillis(totalTimeoutMillis))
.build())
.build()
.getStorageSettings();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,16 +55,16 @@
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.nio.ByteBuffer;
import java.time.Clock;
import java.time.Instant;
import java.time.ZoneId;
import java.time.ZoneOffset;
import java.time.format.DateTimeFormatter;
import java.util.Arrays;
import java.util.Optional;
import java.util.logging.Logger;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.threeten.bp.Clock;
import org.threeten.bp.Instant;
import org.threeten.bp.ZoneId;
import org.threeten.bp.ZoneOffset;
import org.threeten.bp.format.DateTimeFormatter;

@RunWith(StorageITRunner.class)
@SingleBackend(Backend.TEST_BENCH)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,11 @@
import com.google.cloud.storage.it.runner.annotations.Backend;
import com.google.cloud.storage.it.runner.annotations.CrossRun;
import com.google.cloud.storage.it.runner.annotations.Inject;
import java.time.Duration;
import java.time.Instant;
import java.util.stream.StreamSupport;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.threeten.bp.Duration;
import org.threeten.bp.Instant;

@RunWith(StorageITRunner.class)
@CrossRun(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,14 +53,14 @@
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
import java.time.Duration;
import java.util.List;
import java.util.Optional;
import java.util.concurrent.TimeUnit;
import java.util.logging.Level;
import java.util.logging.Logger;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import org.threeten.bp.Duration;

/**
* A {@link ManagedLifecycle} which integrates with the <a target="_blank"
Expand Down Expand Up @@ -260,10 +260,10 @@ public void start() {
runWithRetries(
TestBench.this::listRetryTests,
RetrySettings.newBuilder()
.setTotalTimeout(Duration.ofSeconds(30))
.setInitialRetryDelay(Duration.ofMillis(500))
.setTotalTimeoutDuration(Duration.ofSeconds(30))
.setInitialRetryDelayDuration(Duration.ofMillis(500))
.setRetryDelayMultiplier(1.5)
.setMaxRetryDelay(Duration.ofSeconds(5))
.setMaxRetryDelayDuration(Duration.ofSeconds(5))
.build(),
new BasicResultRetryAlgorithm<List<RetryTestResource>>() {
@Override
Expand Down Expand Up @@ -335,10 +335,10 @@ public void stop() {
throw new NotShutdownException();
},
RetrySettings.newBuilder()
.setTotalTimeout(Duration.ofSeconds(30))
.setInitialRetryDelay(Duration.ofMillis(500))
.setTotalTimeoutDuration(Duration.ofSeconds(30))
.setInitialRetryDelayDuration(Duration.ofMillis(500))
.setRetryDelayMultiplier(1.5)
.setMaxRetryDelay(Duration.ofSeconds(5))
.setMaxRetryDelayDuration(Duration.ofSeconds(5))
.build(),
new BasicResultRetryAlgorithm<List<?>>() {
@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import com.google.common.collect.ImmutableList;
import java.io.ByteArrayInputStream;
import java.io.InputStream;
import java.time.Duration;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
Expand All @@ -43,7 +44,6 @@
import org.junit.Before;
import org.junit.Test;
import org.mockito.Mockito;
import org.threeten.bp.Duration;

public class RemoteStorageHelperTest {

Expand Down Expand Up @@ -273,8 +273,8 @@ public void testCreateFromStream() {
assertEquals(60000, ((HttpTransportOptions) options.getTransportOptions()).getConnectTimeout());
assertEquals(60000, ((HttpTransportOptions) options.getTransportOptions()).getReadTimeout());
assertEquals(10, options.getRetrySettings().getMaxAttempts());
assertEquals(Duration.ofMillis(30000), options.getRetrySettings().getMaxRetryDelay());
assertEquals(Duration.ofMillis(120000), options.getRetrySettings().getTotalTimeout());
assertEquals(Duration.ofMillis(250), options.getRetrySettings().getInitialRetryDelay());
assertEquals(Duration.ofMillis(30000), options.getRetrySettings().getMaxRetryDelayDuration());
assertEquals(Duration.ofMillis(120000), options.getRetrySettings().getTotalTimeoutDuration());
assertEquals(Duration.ofMillis(250), options.getRetrySettings().getInitialRetryDelayDuration());
}
}
Loading