-
Notifications
You must be signed in to change notification settings - Fork 53
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 to Gax-Java #1872
Conversation
java.time
in gaxjava.time
in gax
…java Co-authored-by: Mike Eltsufin <[email protected]>
…java Co-authored-by: Mike Eltsufin <[email protected]>
Quality Gate failed for 'gapic-generator-java-root'Failed conditions See analysis details on SonarCloud Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
Quality Gate failed for 'java_showcase_integration_tests'Failed conditions See analysis details on SonarCloud Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
The java-spanner downstream check is failing because a new method
The test cc: @rahul2393 |
🤖 I have created a release *beep* *boop* --- <details><summary>2.43.0</summary> ## [2.43.0](v2.42.0...v2.43.0) (2024-07-25) ### Features * add `transport` option to `generation_config.yaml` ([#3052](#3052)) ([3b1a915](3b1a915)) * get released version from versions.txt to render `README.md` ([#3007](#3007)) ([99bb2b3](99bb2b3)) * Introduce java.time to Gax-Java ([#1872](#1872)) ([308aeaf](308aeaf)) * Mark `getDefaultEndpoint()` with @ObsoleteApi ([#2347](#2347)) ([e46648f](e46648f)) * parse `BUILD.bzel` to determine whether a commit that only changed `BUILD.bazel` is a qualified commit ([#2937](#2937)) ([502f801](502f801)) ### Bug Fixes * Fix: ([d996c2d](d996c2d)) * `BaseApiTracer` to noop on attemptFailed via overloaded method call ([#3016](#3016)) ([2fc938a](2fc938a)) * Generator to skip generation for empty services. ([#3051](#3051)) ([ff2c485](ff2c485)) * restore hermetic build image publication ([#2952](#2952)) ([97a6d67](97a6d67)) ### Dependencies * update dependency com.fasterxml.jackson:jackson-bom to v2.17.2 ([#3028](#3028)) ([d16f9d1](d16f9d1)) * update dependency com.google.cloud.opentelemetry:detector-resources-support to v0.30.0 ([#2975](#2975)) ([b3ec93f](b3ec93f)) * update dependency com.google.cloud.opentelemetry:detector-resources-support to v0.31.0 ([#3044](#3044)) ([6bd07dc](6bd07dc)) * update dependency com.google.errorprone:error_prone_annotations to v2.29.2 ([#3058](#3058)) ([8ea0868](8ea0868)) * update dependency com.google.errorprone:error_prone_annotations to v2.29.2 ([#3059](#3059)) ([81b23dc](81b23dc)) * update dependency com.google.guava:guava to v33.2.1-jre ([#3027](#3027)) ([12ee456](12ee456)) * update dependency commons-codec:commons-codec to v1.17.1 ([#3049](#3049)) ([58d94b7](58d94b7)) * update dependency dev.cel:cel to v0.6.0 ([#3050](#3050)) ([bc332d9](bc332d9)) * update dependency net.bytebuddy:byte-buddy to v1.14.18 ([#3029](#3029)) ([8799cf6](8799cf6)) * update dependency org.apache.commons:commons-lang3 to v3.15.0 ([#3060](#3060)) ([2538334](2538334)) * update dependency org.checkerframework:checker-qual to v3.45.0 ([#2988](#2988)) ([4edd216](4edd216)) * update google api dependencies ([#2951](#2951)) ([c16f6c9](c16f6c9)) * update google auth library dependencies to v1.24.0 ([#3039](#3039)) ([98b5bd7](98b5bd7)) * update googleapis/java-cloud-bom digest to 47c5dbc ([#2974](#2974)) ([57623f0](57623f0)) * update grpc dependencies to v1.65.1 ([#3061](#3061)) ([27497e2](27497e2)) * update junit5 monorepo to v5.10.3 ([#2963](#2963)) ([bc55fe1](bc55fe1)) * update netty dependencies to v4.1.112.final ([#3057](#3057)) ([5af127b](5af127b)) * update opentelemetry-java monorepo to v1.40.0 ([#3035](#3035)) ([5c31c42](5c31c42)) * Use Gapic-Showcase v0.35.1 ([#3018](#3018)) ([43773f0](43773f0)) ### Documentation * add support option to 'new issue' choices ([#3055](#3055)) ([6a2a17d](6a2a17d)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
if (source == null) { | ||
return null; | ||
} | ||
return java.time.Instant.ofEpochMilli(source.toEpochMilli()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, this truncates sub-millisecond precision. I think what you probably want to do is instead:
return java.time.Instant.ofEpochSecond(source.getEpochSecond(), source.getNano());
Ditto on the other methods in this file (including converting via nanoseconds for the Duration method --- that will overflow for values > 292 years --- instead, use Duration.ofSeconds(duration.getSeconds(), duration.getNano());
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CC: @diegomarquezp Can you take a look at this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I confirmed this causes trouble:
@Test
void testToThreeteenTimeInstant_bigInput_doesNotTruncate() {
java.time.Instant jtInstant = java.time.Instant.MAX;
org.threeten.bp.Instant ttInstant = TimeConversionUtils.toThreetenInstant(jtInstant);
assertEquals(jtInstant.toEpochMilli(), ttInstant.toEpochMilli());
}
It did not truncate, though. It failed with an overflow exception:
java.lang.ArithmeticException: long overflow
at java.base/java.lang.Math.multiplyExact(Math.java:1033)
at java.base/java.lang.Math.multiplyExact(Math.java:1009)
at java.base/java.time.Instant.toEpochMilli(Instant.java:1262)
at com.google.api.gax.util.TimeConversionUtils.toThreetenInstant(TimeConversionUtils.java:59)
I'll leave this test as a standard for a coming PR.
) #1872 (comment) --------- Co-authored-by: Lawrence Qiu <[email protected]>
) #1872 (comment) --------- Co-authored-by: Lawrence Qiu <[email protected]>
…methods (#3321) This PR leverages the changes in #1872 so the generated code uses the new `java.time` methods. All test-related changes are golden file/string changes. #### SonarCloud complaints The SonarCloud scanner complains about code duplication in the generator code, although after inspection I could not find a reason for stating so. It also complains about showcase although the changes in this folder are strictly golden-oriented. #### Downstream check failures They are of the form "could not find remote branch `vX.X.X` to checkout". This seems not related to this PR as it is also present in #3318
This introduces the use of
java.time
in replacement oforg.threeten
backport classes.Message to library users (release note)
We have made the following backwards-compatible changes in this library:
org.threeten.bp
tojava.time
. This has no effect in the API surface.java.time
versions for each of the abstract methods involvingorg.threeten.bp
. The affected classes areBatchingSettings
,RetrySettings
,TimedAttemptSettings
andClientContext
, all of which areAutoValue
classes, not meant to be extended by our users.org.threeten.bp
will be marked final because we will force their behavior to be the same one as thejava.time
counterpart, which has the actual implementation. The affected classes areBatchingSettings
,RetrySettings
,TimedAttemptSettings
andClientContext
, all of which areAutoValue
classes, not meant to be extended by our users.interface
s, we have addedjava.time
versions for each of the methods involvingorg.threeten.bp
. The affected interfaces areWatchdogProvider
(public interface),ApiTracer
(internal) andApiCallContext
(internal). The new method inWatchdogProvider
has adefault
behavior, which is to call thethreeten
counterpart, in order to make this a backwards-compatible change.Implementation
It preserves method signatures using
threeten
. The cases are:@AutoValue
clases and classes with public interface involving time objectsAutovalues will have an internal
threeten
variable with a conveniencejava.time
wrapper method. For example:sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/retrying/RetrySettings.java
Lines 78 to 89 in 667f2da
and
sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/retrying/RetrySettings.java
Lines 243 to 262 in a99d3c1
Internal variables
Duration
orInstant
private variables are switched tojava.time
. In this case, the wrappers are ofthreeten
clasess. For examplesdk-platform-java/gax-java/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonCallContext.java
Lines 237 to 247 in 667f2da
Deprecation warning
threeten
methods will now have an@ObsoleteApi
annotation. For example:sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/batching/BatchingSettings.java
Line 106 in 667f2da
CLIRR checks
gax-java/gax
We have decided to ignore a these CLIRR messages due to the following reasons
7005
(method type change): This has only been implemented in internal classes, not exposed to our customers7013
(new abstract method added): We have added several java.time versions for each of the methods involving org.threeten.bp. This affectsAutoValue
classes only.7014
(convert to final): All getters and setters using org.threeten.bp will be marked final because we will force their behavior to be the same one as the java.time counterpart, which has the actual implementation. This affectsAutoValue
classes only.7012
(new method in interface): We have added several java.time versions for each of the methods involving org.threeten.bp. For the only public interfaceWatchdogProvider
, we added adefault
behavior.