feat: [wip] `threeten` deprecation in `gax`

[diegomarquezp]

This introduces the use of java.time in replacement of org.threeten backport classes.

Implementation

It preserves method signatures using threeten. The cases are:

AutoValue.Builder

Autovalues will have an internal threeten variable with a convenience java.time wrapper method. For example: https://github.com/googleapis/sdk-platform-java/blob/667f2da15d0800d87d0866e36627ac23955c0b01/gax-java/gax/src/main/java/com/google/api/gax/retrying/RetrySettings.java#L78-L89

and

https://github.com/googleapis/sdk-platform-java/blob/667f2da15d0800d87d0866e36627ac23955c0b01/gax-java/gax/src/main/java/com/google/api/gax/retrying/RetrySettings.java#L200-L211

Internal variables

Duration or Instant private variables are switched to java.time. In this case, the wrappers are of threeten clasess. For example https://github.com/googleapis/sdk-platform-java/blob/667f2da15d0800d87d0866e36627ac23955c0b01/gax-java/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonCallContext.java#L237-L247

Deprecation warning

threeten methods will now have an @ObsoleteApi annotation. For example: https://github.com/googleapis/sdk-platform-java/blob/667f2da15d0800d87d0866e36627ac23955c0b01/gax-java/gax/src/main/java/com/google/api/gax/batching/BatchingSettings.java#L106

CLIRR checks

gax-java/gax

[ERROR] 7005: com.google.api.gax.retrying.DirectRetryingExecutor: Parameter 1 of 'protected void sleep(org.threeten.bp.Duration)' has changed its type to java.time.Duration
[ERROR] 7012: com.google.api.gax.rpc.ApiCallContext: Method 'public java.time.Duration getStreamIdleTimeoutDuration()' has been added to an interface
[ERROR] 7012: com.google.api.gax.rpc.ApiCallContext: Method 'public java.time.Duration getStreamWaitTimeoutDuration()' has been added to an interface
[ERROR] 7012: com.google.api.gax.rpc.ApiCallContext: Method 'public java.time.Duration getTimeoutDuration()' has been added to an interface
[ERROR] 7012: com.google.api.gax.rpc.ApiCallContext: Method 'public com.google.api.gax.rpc.ApiCallContext withStreamIdleTimeout(java.time.Duration)' has been added to an interface
[ERROR] 7012: com.google.api.gax.rpc.ApiCallContext: Method 'public com.google.api.gax.rpc.ApiCallContext withStreamWaitTimeout(java.time.Duration)' has been added to an interface
[ERROR] 7012: com.google.api.gax.rpc.ApiCallContext: Method 'public com.google.api.gax.rpc.ApiCallContext withTimeout(java.time.Duration)' has been added to an interface
[ERROR] 7005: com.google.api.gax.rpc.Watchdog: Parameter 2 of 'public com.google.api.gax.rpc.Watchdog create(com.google.api.core.ApiClock, org.threeten.bp.Duration, java.util.concurrent.ScheduledExecutorService)' has changed its type to java.time.Duration
[ERROR] 7012: com.google.api.gax.rpc.WatchdogProvider: Method 'public com.google.api.gax.rpc.WatchdogProvider withCheckInterval(java.time.Duration)' has been added to an interface
[ERROR] 7005: com.google.api.gax.tracing.ApiTracer: Parameter 2 of 'public void attemptFailed(java.lang.Throwable, org.threeten.bp.Duration)' has changed its type to java.time.Duration
[ERROR] 7005: com.google.api.gax.tracing.BaseApiTracer: Parameter 2 of 'public void attemptFailed(java.lang.Throwable, org.threeten.bp.Duration)' has changed its type to java.time.Duration
[ERROR] 7005: com.google.api.gax.tracing.OpencensusTracer: Parameter 2 of 'public void attemptFailed(java.lang.Throwable, org.threeten.bp.Duration)' has changed its type to java.time.Duration

google-cloud-core

[ERROR] 7005: com.google.cloud.testing.BaseEmulatorHelper: Parameter 1 of 'public void stop(org.threeten.bp.Duration)' has changed its type to java.time.Duration
[ERROR] 7005: com.google.cloud.testing.BaseEmulatorHelper: Parameter 1 of 'protected int waitForProcess(org.threeten.bp.Duration)' has changed its type to java.time.Duration
[ERROR] 7005: com.google.cloud.testing.BaseEmulatorHelper$DownloadableEmulatorRunner: Parameter 1 of 'public int waitFor(org.threeten.bp.Duration)' has changed its type to java.time.Duration
[ERROR] 7005: com.google.cloud.testing.BaseEmulatorHelper$EmulatorRunner: Parameter 1 of 'public int waitFor(org.threeten.bp.Duration)' has changed its type to java.time.Duration
[ERROR] 7005: com.google.cloud.testing.BaseEmulatorHelper$GcloudEmulatorRunner: Parameter 1 of 'public int waitFor(org.threeten.bp.Duration)' has changed its type to java.time.Duration

Pending

• [ ] add import java.time.Duration/Instant to classes

[sonarqubecloud[bot]]

[gapic-generator-java-root] SonarCloud Quality Gate failed.   

1 Bug
0 Vulnerabilities
0 Security Hotspots
12 Code Smells

70.7% Coverage
4.2% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

[sonarqubecloud[bot]]

[java_showcase_integration_tests] SonarCloud Quality Gate failed.   

1 Bug
0 Vulnerabilities
0 Security Hotspots
10 Code Smells

50.9% Coverage
4.2% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

[diegomarquezp]

Some downstreams are failing. At least this one in pubsub fails due to a null value passed to a overloaded setter. @blakeli0 is this some expected work on the hanwritten libraries? (this may also occur in user code)

https://github.com/googleapis/java-pubsub/blob/88d5968862c54327a2538e7b0fd611fd131cb6cc/google-cloud-pubsub/src/test/java/com/google/cloud/pubsub/v1/PublisherImplTest.java#L900-L904

[lqiu96]

Autovalues will have an internal threeten variable with a convenience java.time wrapper method.

Might be missing some details, but why not have an internal java.time variable and the existing threeten methods wrap to the new java.time wrapper methods? I think it would be easier to remove the threeten methods from autovalue in the future since the java.time methods wouldn't need to be rewritten.

[lqiu96]

For the PR title, can it be changed from feat: [wip] threeten deprecation in gax to feat: Introduce java.time to Gax-Java since we are not actually deprecating any methods yet?

[diegomarquezp]

Autovalues will have an internal threeten variable with a convenience java.time wrapper method.

Might be missing some details, but why not have an internal java.time variable and the existing threeten methods wrap to the new java.time wrapper methods? I think it would be easier to remove the threeten methods from autovalue in the future since the java.time methods wouldn't need to be rewritten.

@lqiu96 that's a good point. Unfortunately I don't have the rationale handy for this. @blakeli0 WDYT? I'll make the change anyways since we can roll back to this point.

[diegomarquezp]

The java-bigtable downstream is flaky in other PRs as well. I created https://github.com/googleapis/java-bigtable/issues/2230

[sonarqubecloud[bot]]

Quality Gate failed for 'gapic-generator-java-root'

Failed conditions
23.6% Coverage on New Code (required ≥ 80%)
B Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

[sonarqubecloud[bot]]

Quality Gate failed for 'java_showcase_integration_tests'

Failed conditions
53.3% Coverage on New Code (required ≥ 80%)
B Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

[suztomo]

In the pull request description, would you add a section "Message to library users (release note)"?

What's rationale for CLIRR check failures? (Can you add the rationale "why the CLIRR errors are not breaking changes to users" in the pull request description?)

[diegomarquezp]

In the pull request description, would you add a section "Message to library users (release note)"?

What's rationale for CLIRR check failures? (Can you add the rationale "why the CLIRR errors are not breaking changes to users" in the pull request description?)

@suztomo I added a note at the end of the PR description.

[diegomarquezp]

BatchingSettings' new methods are covered with a new test suite BatchingSettingsTest Example ci job:

[INFO] Running com.google.api.gax.batching.BatchingSettingsTest
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.001 s -- in com.google.api.gax.batching.BatchingSettingsTest

However SonarCloud doesn't show it as hit

Could this be because the way we call these methods is via lambdas? https://github.com/googleapis/sdk-platform-java/blob/76048973699525d7ff51cfd763f1947424096674/gax-java/gax/src/test/java/com/google/api/gax/batching/BatchingSettingsTest.java#L41-L48

[lqiu96]

@diegomarquezp @blakeli0 I think I found the root cause of the sonar issues listed in https://github.com/googleapis/sdk-platform-java/issues/2520.

I believe it's due to this line in the surefire maven plugin for the gax module: https://github.com/googleapis/sdk-platform-java/blob/74f20a253e65d9d4253541c4c8250d75abe8d0b8/gax-java/gax/pom.xml#L106

Jacoco Coverage plugin injects a java agent which I believe is added into surefire's argline. If you set a custom value in the pom, then surefire will not read the property and coverage won't be run for the module.

From the logs of the previous sonar run: The agent looks to be injected into the gax module, but it is not run for the module:

I followed this blog to preserve the existing surefire argline params and added it to this PR to test if coverage would be fixed. I believe the code coverage percentage is back to a normal-ish percentage.

Also, I took a look at the PR which introduced it: https://github.com/googleapis/sdk-platform-java/pull/2285. It was added on Dec 6th and it looks like the first occurance where it sonar code coverage percentage dropped is in a PR on the next day: https://github.com/googleapis/sdk-platform-java/pull/2275

[diegomarquezp]

@diegomarquezp @blakeli0 I think I found the root cause of the sonar issues listed in #2520.

I believe it's due to this line in the surefire maven plugin for the gax module:

https://github.com/googleapis/sdk-platform-java/blob/74f20a253e65d9d4253541c4c8250d75abe8d0b8/gax-java/gax/pom.xml#L106

Jacoco Coverage plugin injects a java agent which I believe is added into surefire's argline. If you set a custom value in the pom, then surefire will not read the property and coverage won't be run for the module.

From the logs of the previous sonar run: The agent looks to be injected into the gax module, but it is not run for the module:

I followed this blog to preserve the existing surefire argline params and added it to this PR to test if coverage would be fixed. I believe the code coverage percentage is back to a normal-ish percentage.

Also, I took a look at the PR which introduced it: #2285. It was added on Dec 6th and it looks like the first occurance where it sonar code coverage percentage dropped is in a PR on the next day: #2275

@lqiu96 the coverage indeed looks closer to what we expected. Thank you!

[sonarqubecloud[bot]]

Quality Gate failed for 'gapic-generator-java-root'

Failed conditions
B Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

[sonarqubecloud[bot]]

Quality Gate failed for 'java_showcase_integration_tests'

Failed conditions
49.8% Coverage on New Code (required ≥ 80%)
B Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

[diegomarquezp]

The java-spanner downstream check is failing because a new method attemptFailedDuration was added to BaseApiTracer.

Error:  Failures: 
Error:    CompositeTracerTest.testMethodsOverrideMetricsTracer:238 Method not found in compositeTracerMethods: public void com.google.api.gax.tracing.MetricsTracer.attemptFailedDuration(java.lang.Throwable,java.time.Duration)

The test CompositeTracerTest.testMethodsOverrideMetricsTracer verifies that the class CompositeTracer has an overridden declaration of each method of BaseApiTracer, which is not true as of July 4th.

cc: @rahul2393