feat: introduce java.time variables and methods #3495

diegomarquezp · 2024-11-21T20:32:52Z

This PR introduces java.time alternatives to existing org.threeten.bp.* methods, as well as switching internal variables (if any) to java.time

The main constraint is to keep the changes backwards compatible, so for each existing threeten method "method1(org.threeten.bp.Duration)" we will add an alternative with a Duration (or Timestamp when applicable) suffix: "method1Duration(java.time.Duration)".

For most cases, the implementation will be held in the java.time method and the old threeten method will just delegate the call to it. However, for the case of abstract classes, the implementation will be kept in the threeten method to avoid breaking changes (i.e. users that already overloaded the method in their user code).

google-cloud-spanner/src/main/java/com/google/cloud/spanner/CompositeTracer.java

google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPoolOptions.java

google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/SpannerRpc.java

sakthivelmanii · 2024-11-26T05:56:49Z

...e-cloud-spanner/src/main/java/com/google/cloud/spanner/MultiplexedSessionDatabaseClient.java

@@ -275,7 +275,7 @@ public void onSessionCreateFailure(Throwable t, int createFailureForSessionCount

  private static void maybeWaitForSessionCreation(
      SessionPoolOptions sessionPoolOptions, ApiFuture<SessionReference> future) {
-    org.threeten.bp.Duration waitDuration = sessionPoolOptions.getWaitForMinSessions();
+    java.time.Duration waitDuration = sessionPoolOptions.getWaitForMinSessions();


nit: java.time.Duration is already imported in this class. Can't we just remove org.threeten.bp.

sakthivelmanii · 2024-11-26T05:58:56Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/CompositeTracer.java

@@ -109,7 +108,7 @@ public void attemptCancelled() {
  }

  @Override
-  public void attemptFailed(Throwable error, Duration delay) {
+  public void attemptFailed(Throwable error, org.threeten.bp.Duration delay) {


nit: Can we import java.time.Duration so that we can remove java.time. in attemptFailedDuration method? https://github.com/googleapis/java-spanner/blob/main/google-cloud-spanner/src/main/java/com/google/cloud/spanner/CompositeTracer.java#L119

sakthivelmanii · 2024-11-26T06:01:35Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/OpenTelemetryApiTracer.java

  @Override
-  public void attemptFailed(Throwable error, Duration delay) {
+  public void attemptFailedDuration(Throwable error, java.time.Duration delay) {


nit: Can we import java.time.Duration?

sakthivelmanii · 2024-11-26T06:03:16Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPoolOptions.java

@@ -63,8 +66,8 @@ public class SessionPoolOptions {
  @Deprecated private final long initialWaitForSessionTimeoutMillis;

  private final boolean autoDetectDialect;
-  private final Duration waitForMinSessions;
-  private final Duration acquireSessionTimeout;
+  private final java.time.Duration waitForMinSessions;


nit: We can import java.time.Duration so that in future we don't have a confusion on which class to use

and also it avoids so many changes in the code which is not really necessary.

sakthivelmanii · 2024-11-26T06:06:57Z

@diegomarquezp Can we make sure following in all the files?

import java.time.Duration in all the files(whereever is required) and use org.threeten.bp.Duration directly in obsolete methods. This avoids lot of code changes which is unnecessary. This also provides the standard that java.time.Duration should be used.

diegomarquezp · 2024-11-27T02:50:40Z

@diegomarquezp Can we make sure following in all the files?

import java.time.Duration in all the files(whereever is required) and use org.threeten.bp.Duration directly in obsolete methods. This avoids lot of code changes which is unnecessary. This also provides the standard that java.time.Duration should be used.

@sakthivelmanii Thanks for the review! Before switching to import java.time.Duration I'd like to double check if our rationale for not importing java.time.* seems sound to you: this is mainly to make it clear to the reader that there are two Duration/Instant classes coexisting in the same file and attention should be paid. We deliberately added these changes just to make this differentiation as explicit as possible. We've been doing it this way in other repos (e.g. googleapis/java-pubsub#2271 (comment)) (doesn't mean it should be done as in other repos).

Promoting java.time as the standard also makes sense, and it's ultimately the Spanner team's call, but wanted to make sure our approach doesn't come up as unintended. I'm happy to move forward either way.

diegomarquezp · 2024-12-03T16:59:12Z

google-cloud-spanner/src/main/java/com/google/cloud/spanner/connection/StatementExecutor.java

@@ -106,26 +106,26 @@ public boolean hasDuration() {
          });
    }

-    org.threeten.bp.Duration asDuration() {
+    java.time.Duration asDuration() {


need to keep this one in favor of com.google.protobuf.Duration

google-cloud-spanner/src/main/java/com/google/cloud/spanner/CompositeTracer.java

lqiu96

LGTM. Spanner team for approval.

diegomarquezp added 2 commits November 21, 2024 14:44

feat: introduce java.time variables and methods

439e105

fix units

fafe22a

diegomarquezp requested a review from a team as a code owner November 21, 2024 20:32

product-auto-label bot added the size: l Pull request size is large. label Nov 21, 2024

diegomarquezp marked this pull request as draft November 21, 2024 20:32

product-auto-label bot added the api: spanner Issues related to the googleapis/java-spanner API. label Nov 21, 2024

remove threeten from executor lib

b421084

diegomarquezp added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 21, 2024

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 21, 2024

diegomarquezp marked this pull request as ready for review November 21, 2024 22:50

diegomarquezp requested a review from a team as a code owner November 21, 2024 22:50

diegomarquezp requested a review from lqiu96 November 21, 2024 22:50

lqiu96 reviewed Nov 21, 2024

View reviewed changes

google-cloud-spanner/src/main/java/com/google/cloud/spanner/CompositeTracer.java Show resolved Hide resolved

lqiu96 reviewed Nov 21, 2024

View reviewed changes

google-cloud-spanner/src/main/java/com/google/cloud/spanner/SessionPoolOptions.java Outdated Show resolved Hide resolved

lqiu96 reviewed Nov 22, 2024

View reviewed changes

google-cloud-spanner/src/main/java/com/google/cloud/spanner/spi/v1/SpannerRpc.java Show resolved Hide resolved

diegomarquezp requested a review from lqiu96 November 22, 2024 19:11

use javatime implementation in tracer

ea066d9

sakthivelmanii reviewed Nov 26, 2024

View reviewed changes

diegomarquezp requested a review from sakthivelmanii November 27, 2024 23:54

use java.time as import

7b77a79

diegomarquezp commented Dec 3, 2024

View reviewed changes

diegomarquezp requested a review from rahul2393 December 4, 2024 18:05

lqiu96 approved these changes Dec 4, 2024

View reviewed changes

rahul2393 approved these changes Dec 5, 2024

View reviewed changes

Merge branch 'main' into introduce-java-time

ade9220

diegomarquezp added the automerge Merge the pull request once unit tests and other checks pass. label Dec 5, 2024

gcf-merge-on-green bot merged commit 8a7d533 into main Dec 5, 2024
34 checks passed

gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Dec 5, 2024

gcf-merge-on-green bot deleted the introduce-java-time branch December 5, 2024 17:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: introduce java.time variables and methods #3495

feat: introduce java.time variables and methods #3495

diegomarquezp commented Nov 21, 2024

sakthivelmanii Nov 26, 2024

sakthivelmanii Nov 26, 2024 •

edited

Loading

diegomarquezp Dec 3, 2024

sakthivelmanii Nov 26, 2024

diegomarquezp Dec 3, 2024

sakthivelmanii Nov 26, 2024

sakthivelmanii Nov 26, 2024

diegomarquezp Dec 3, 2024

sakthivelmanii commented Nov 26, 2024 •

edited

Loading

diegomarquezp commented Nov 27, 2024 •

edited

Loading

diegomarquezp Dec 3, 2024

lqiu96 left a comment

feat: introduce java.time variables and methods #3495

feat: introduce java.time variables and methods #3495

Conversation

diegomarquezp commented Nov 21, 2024

Choose a reason for hiding this comment

sakthivelmanii Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sakthivelmanii commented Nov 26, 2024 • edited Loading

diegomarquezp commented Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

lqiu96 left a comment

Choose a reason for hiding this comment

sakthivelmanii Nov 26, 2024 •

edited

Loading

sakthivelmanii commented Nov 26, 2024 •

edited

Loading

diegomarquezp commented Nov 27, 2024 •

edited

Loading