Skip to content

Commit

Permalink
feat: introduce java.time to java-core (#3330)
Browse files Browse the repository at this point in the history
## In this PR:
* Modify publicly exposed methods to offer a `java.time` alternative
(suffixed with `Duration`). These methods will now hold the actual
implementation, whereas the old signatures will encapsulate the new
ones. Example: `retryDelay(org.threeten.bp.Duration)` encapsulates
`retryDelayDuration(java.time.Duration)`


## Notes
### _CLIRR_
```
[ERROR] 7012: com.google.cloud.testing.BaseEmulatorHelper$EmulatorRunner: Method 'public int waitForDuration(java.time.Duration)' has been added to an interface
```
This new interface method was added. However, we ignore this change
alert because the method has a `default` implementation.

### Addressing a behavior change in `Timestamp`
#### Problem
When using java.time functions, parsing a datetime string with offset
produces a wrong time object (offset is not added to the effective epoch
seconds).

#### Context
Full context in
https://github.com/googleapis/sdk-platform-java/pull/3330/files#r1828424787


adoptium/jdk@c6d209b
was introduced as a fix (in Java 9) meant for [this
issue](https://bugs.openjdk.org/browse/JDK-8066982).
In java 8, this means that the offset value is stored separately in a
variable that is not respected by the parsing functions before this fix.
The workaround is to use `appendZoneOrOffsetId()`, which stores the
offset value in the `zone` variable of a parsing context, which is
[respected as of java
8](https://github.com/adoptium/jdk8u/blob/31b88042fba46e87fba83e8bfd43ae0ecb5a9afd/jdk/src/share/classes/java/time/format/Parsed.java#L589-L591).

Additionally, under the consideration of this having unwanted side
effects, we expanded the test suite to test for more edge and normal
cases using an offset string. We also
[searched](https://bugs.openjdk.org/browse/JDK-8202948?jql=affectedVersion%20%3D%20%228%22%20AND%20text%20~%20%22offset%22)
the JDK's issue tracking database and found somewhat similar issues with
parsing and offsets but no workaround that addressed our specific
situation. This is also cause of no backports of the
[fix](adoptium/jdk@c6d209b)
for Java 9 into Java 8.

#### Outcome
The behavior is kept the same as stated by our tests and downstream
checks
  • Loading branch information
diegomarquezp authored Nov 14, 2024
1 parent c624b89 commit f202c3b
Show file tree
Hide file tree
Showing 9 changed files with 247 additions and 59 deletions.
6 changes: 6 additions & 0 deletions java-core/google-cloud-core/clirr-ignored-differences.xml
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,10 @@
<className>com/google/cloud/ReadChannel</className>
<method>long limit()</method>
</difference>
<difference>
<!-- Default method added to interface -->
<differenceType>7012</differenceType>
<className>com/google/cloud/testing/BaseEmulatorHelper$EmulatorRunner</className>
<method>int waitForDuration(java.time.Duration)</method>
</difference>
</differences>
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,13 @@

package com.google.cloud;

import static com.google.api.gax.util.TimeConversionUtils.toJavaTimeDuration;
import static com.google.common.base.Preconditions.checkNotNull;

import com.google.api.core.BetaApi;
import com.google.api.core.ObsoleteApi;
import com.google.api.gax.retrying.RetrySettings;
import java.io.Serializable;
import org.threeten.bp.Duration;

/**
* This class represents an options wrapper around the {@link RetrySettings} class and is an
Expand Down Expand Up @@ -51,13 +52,25 @@ private RetryOption(OptionType type, Object value) {
this.value = checkNotNull(value);
}

/** See {@link RetrySettings#getTotalTimeout()}. */
public static RetryOption totalTimeout(Duration totalTimeout) {
/** This method is obsolete. Use {@link #totalTimeoutDuration(java.time.Duration)} instead */
@ObsoleteApi("Use totalTimeouDuration() instead")
public static RetryOption totalTimeout(org.threeten.bp.Duration totalTimeout) {
return totalTimeoutDuration(toJavaTimeDuration(totalTimeout));
}

/** See {@link RetrySettings#getTotalTimeoutDuration()}. */
public static RetryOption totalTimeoutDuration(java.time.Duration totalTimeout) {
return new RetryOption(OptionType.TOTAL_TIMEOUT, totalTimeout);
}

/** See {@link RetrySettings#getInitialRetryDelay()}. */
public static RetryOption initialRetryDelay(Duration initialRetryDelay) {
/** This method is obsolete. Use {@link #initialRetryDelayDuration(java.time.Duration)} instead */
@ObsoleteApi("Use initialRetryDelayDuration() instead")
public static RetryOption initialRetryDelay(org.threeten.bp.Duration initialRetryDelay) {
return initialRetryDelayDuration(toJavaTimeDuration(initialRetryDelay));
}

/** See {@link RetrySettings#getInitialRetryDelayDuration()}. */
public static RetryOption initialRetryDelayDuration(java.time.Duration initialRetryDelay) {
return new RetryOption(OptionType.INITIAL_RETRY_DELAY, initialRetryDelay);
}

Expand All @@ -66,8 +79,14 @@ public static RetryOption retryDelayMultiplier(double retryDelayMultiplier) {
return new RetryOption(OptionType.RETRY_DELAY_MULTIPLIER, retryDelayMultiplier);
}

/** See {@link RetrySettings#getMaxRetryDelay()}. */
public static RetryOption maxRetryDelay(Duration maxRetryDelay) {
/** This method is obsolete. Use {@link #maxRetryDelayDuration(java.time.Duration)} instead */
@ObsoleteApi("Use maxRetryDelayDuration() instead")
public static RetryOption maxRetryDelay(org.threeten.bp.Duration maxRetryDelay) {
return maxRetryDelayDuration(toJavaTimeDuration(maxRetryDelay));
}

/** See {@link RetrySettings#getMaxRetryDelayDuration()}. */
public static RetryOption maxRetryDelayDuration(java.time.Duration maxRetryDelay) {
return new RetryOption(OptionType.MAX_RETRY_DELAY, maxRetryDelay);
}

Expand Down Expand Up @@ -124,16 +143,16 @@ public static RetrySettings mergeToSettings(RetrySettings settings, RetryOption.
for (RetryOption option : options) {
switch (option.type) {
case TOTAL_TIMEOUT:
builder.setTotalTimeout((Duration) option.value);
builder.setTotalTimeoutDuration((java.time.Duration) option.value);
break;
case INITIAL_RETRY_DELAY:
builder.setInitialRetryDelay((Duration) option.value);
builder.setInitialRetryDelayDuration((java.time.Duration) option.value);
break;
case RETRY_DELAY_MULTIPLIER:
builder.setRetryDelayMultiplier((Double) option.value);
break;
case MAX_RETRY_DELAY:
builder.setMaxRetryDelay((Duration) option.value);
builder.setMaxRetryDelayDuration((java.time.Duration) option.value);
break;
case MAX_ATTEMPTS:
builder.setMaxAttempts((Integer) option.value);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,14 +63,14 @@
import java.io.Serializable;
import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;
import java.time.Duration;
import java.util.Locale;
import java.util.Map;
import java.util.Objects;
import java.util.ServiceLoader;
import java.util.Set;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import org.threeten.bp.Duration;

/**
* Abstract class representing service options.
Expand Down Expand Up @@ -787,13 +787,13 @@ public static RetrySettings getNoRetrySettings() {
private static RetrySettings.Builder getDefaultRetrySettingsBuilder() {
return RetrySettings.newBuilder()
.setMaxAttempts(6)
.setInitialRetryDelay(Duration.ofMillis(1000L))
.setMaxRetryDelay(Duration.ofMillis(32_000L))
.setInitialRetryDelayDuration(Duration.ofMillis(1000L))
.setMaxRetryDelayDuration(Duration.ofMillis(32_000L))
.setRetryDelayMultiplier(2.0)
.setTotalTimeout(Duration.ofMillis(50_000L))
.setInitialRpcTimeout(Duration.ofMillis(50_000L))
.setTotalTimeoutDuration(Duration.ofMillis(50_000L))
.setInitialRpcTimeoutDuration(Duration.ofMillis(50_000L))
.setRpcTimeoutMultiplier(1.0)
.setMaxRpcTimeout(Duration.ofMillis(50_000L));
.setMaxRpcTimeoutDuration(Duration.ofMillis(50_000L));
}

protected abstract Set<String> getScopes();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,19 @@

import static com.google.common.base.Preconditions.checkArgument;

import com.google.api.core.ObsoleteApi;
import com.google.protobuf.util.Timestamps;
import java.io.Serializable;
import java.time.Instant;
import java.time.LocalDateTime;
import java.time.ZoneOffset;
import java.time.format.DateTimeFormatter;
import java.time.format.DateTimeFormatterBuilder;
import java.time.format.DateTimeParseException;
import java.time.temporal.TemporalAccessor;
import java.util.Date;
import java.util.Objects;
import java.util.concurrent.TimeUnit;
import org.threeten.bp.Instant;
import org.threeten.bp.LocalDateTime;
import org.threeten.bp.ZoneOffset;
import org.threeten.bp.format.DateTimeFormatter;
import org.threeten.bp.format.DateTimeFormatterBuilder;
import org.threeten.bp.format.DateTimeParseException;
import org.threeten.bp.temporal.TemporalAccessor;

/**
* Represents a timestamp with nanosecond precision. Timestamps cover the range [0001-01-01,
Expand All @@ -54,7 +55,7 @@ public final class Timestamp implements Comparable<Timestamp>, Serializable {
new DateTimeFormatterBuilder()
.appendOptional(DateTimeFormatter.ISO_LOCAL_DATE_TIME)
.optionalStart()
.appendOffsetId()
.appendZoneOrOffsetId()
.optionalEnd()
.toFormatter()
.withZone(ZoneOffset.UTC);
Expand Down Expand Up @@ -189,6 +190,17 @@ public com.google.protobuf.Timestamp toProto() {
return com.google.protobuf.Timestamp.newBuilder().setSeconds(seconds).setNanos(nanos).build();
}

/** This method is obsolete. Use {@link #parseTimestampDuration(String)} instead */
@ObsoleteApi("Use parseTimestampDuration(String) instead")
public static Timestamp parseTimestamp(String timestamp) {
try {
return parseTimestampDuration(timestamp);
} catch (DateTimeParseException ex) {
throw new org.threeten.bp.format.DateTimeParseException(
ex.getMessage(), ex.getParsedString(), ex.getErrorIndex());
}
}

/**
* Creates a Timestamp instance from the given string. Input string should be in the RFC 3339
* format, like '2020-12-01T10:15:30.000Z' or with the timezone offset, such as
Expand All @@ -198,7 +210,7 @@ public com.google.protobuf.Timestamp toProto() {
* @return created Timestamp
* @throws DateTimeParseException if unable to parse
*/
public static Timestamp parseTimestamp(String timestamp) {
public static Timestamp parseTimestampDuration(String timestamp) {
TemporalAccessor temporalAccessor = timestampParser.parse(timestamp);
Instant instant = Instant.from(temporalAccessor);
return ofTimeSecondsAndNanos(instant.getEpochSecond(), instant.getNano());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,12 @@

package com.google.cloud.testing;

import static com.google.api.gax.util.TimeConversionUtils.toJavaTimeDuration;
import static com.google.api.gax.util.TimeConversionUtils.toThreetenDuration;

import com.google.api.core.CurrentMillisClock;
import com.google.api.core.InternalApi;
import com.google.api.core.ObsoleteApi;
import com.google.cloud.ExceptionHandler;
import com.google.cloud.RetryHelper;
import com.google.cloud.ServiceOptions;
Expand Down Expand Up @@ -56,7 +60,6 @@
import java.util.logging.Logger;
import java.util.zip.ZipEntry;
import java.util.zip.ZipInputStream;
import org.threeten.bp.Duration;

/** Utility class to start and stop a local service which is used by unit testing. */
@InternalApi
Expand Down Expand Up @@ -112,14 +115,21 @@ protected final void startProcess(String blockUntilOutput)
}
}

/** This method is obsolete. Use {@link #waitForProcessDuration(java.time.Duration)} instead */
@ObsoleteApi("Use waitForProcessDuration(java.time.Duration) instead")
protected final int waitForProcess(org.threeten.bp.Duration timeout)
throws IOException, InterruptedException, TimeoutException {
return waitForProcessDuration(toJavaTimeDuration(timeout));
}

/**
* Waits for the local service's subprocess to terminate, and stop any possible thread listening
* for its output.
*/
protected final int waitForProcess(Duration timeout)
protected final int waitForProcessDuration(java.time.Duration timeout)
throws IOException, InterruptedException, TimeoutException {
if (activeRunner != null) {
int exitCode = activeRunner.waitFor(timeout);
int exitCode = activeRunner.waitForDuration(timeout);
activeRunner = null;
return exitCode;
}
Expand All @@ -130,7 +140,7 @@ protected final int waitForProcess(Duration timeout)
return 0;
}

private static int waitForProcess(final Process process, Duration timeout)
private static int waitForProcess(final Process process, java.time.Duration timeout)
throws InterruptedException, TimeoutException {
if (process == null) {
return 0;
Expand Down Expand Up @@ -180,10 +190,17 @@ public String getProjectId() {
/** Starts the local emulator. */
public abstract void start() throws IOException, InterruptedException;

/** Stops the local emulator. */
public abstract void stop(Duration timeout)
/** This method is obsolete. Use {@link #stopDuration(java.time.Duration)} instead */
@ObsoleteApi("Use stopDuration() instead")
public abstract void stop(org.threeten.bp.Duration timeout)
throws IOException, InterruptedException, TimeoutException;

/** Stops the local emulator. */
public void stopDuration(java.time.Duration timeout)
throws IOException, InterruptedException, TimeoutException {
stop(toThreetenDuration(timeout));
}

/** Resets the internal state of the emulator. */
public abstract void reset() throws IOException;

Expand Down Expand Up @@ -226,8 +243,15 @@ protected interface EmulatorRunner {
/** Starts the emulator associated to this runner. */
void start() throws IOException;

/** This method is obsolete. Use {@link #waitForDuration(java.time.Duration)} instead */
@ObsoleteApi("Use waitForDuration() instead")
int waitFor(org.threeten.bp.Duration timeout) throws InterruptedException, TimeoutException;

/** Wait for the emulator associated to this runner to terminate, returning the exit status. */
int waitFor(Duration timeout) throws InterruptedException, TimeoutException;
default int waitForDuration(java.time.Duration timeout)
throws InterruptedException, TimeoutException {
return waitFor(toThreetenDuration(timeout));
}

/** Returns the process associated to the emulator, if any. */
Process getProcess();
Expand Down Expand Up @@ -263,9 +287,17 @@ public void start() throws IOException {
log.fine("Starting emulator via Google Cloud SDK");
process = CommandWrapper.create().setCommand(commandText).setRedirectErrorStream().start();
}
/** This method is obsolete. Use {@link #waitForDuration(java.time.Duration)} instead */
@ObsoleteApi("Use waitForDuration() instead")
@Override
public int waitFor(org.threeten.bp.Duration timeout)
throws InterruptedException, TimeoutException {
return waitForDuration(toJavaTimeDuration(timeout));
}

@Override
public int waitFor(Duration timeout) throws InterruptedException, TimeoutException {
public int waitForDuration(java.time.Duration timeout)
throws InterruptedException, TimeoutException {
return waitForProcess(process, timeout);
}

Expand Down Expand Up @@ -374,8 +406,16 @@ public Path call() throws IOException {
.start();
}

/** This method is obsolete. Use {@link #waitForDuration(java.time.Duration)} instead */
@ObsoleteApi("Use waitForDuration() instead")
@Override
public int waitFor(Duration timeout) throws InterruptedException, TimeoutException {
public int waitFor(org.threeten.bp.Duration timeout)
throws InterruptedException, TimeoutException {
return waitForDuration(toJavaTimeDuration(timeout));
}

public int waitForDuration(java.time.Duration timeout)
throws InterruptedException, TimeoutException {
return waitForProcess(process, timeout);
}

Expand Down
Loading

0 comments on commit f202c3b

Please sign in to comment.