[sdk-platform-java] gax: Consider removing @BetaApi and/or adding @InternalExtensionOnly to retry surface

[burkedavison]

Decide whether to add @InternalExtensionOnly to the following retry elements, and whether to remove @BetaApi

https://github.com/googleapis/sdk-platform-java/blob/decd7f60017d45288e6c12493fa21e06f9c5f77c/gax-java/gax/src/main/java/com/google/api/gax/retrying/ScheduledRetryingExecutor.java#L98

https://github.com/googleapis/sdk-platform-java/blob/decd7f60017d45288e6c12493fa21e06f9c5f77c/gax-java/gax/src/main/java/com/google/api/gax/retrying/RetrySettings.java#L309

https://github.com/googleapis/sdk-platform-java/blob/decd7f60017d45288e6c12493fa21e06f9c5f77c/gax-java/gax/src/main/java/com/google/api/gax/retrying/RetryingContext.java#L44

https://github.com/googleapis/sdk-platform-java/blame/decd7f60017d45288e6c12493fa21e06f9c5f77c/gax-java/gax/src/main/java/com/google/api/gax/retrying/RetryAlgorithm.java#L258

https://github.com/googleapis/sdk-platform-java/blame/decd7f60017d45288e6c12493fa21e06f9c5f77c/gax-java/gax/src/main/java/com/google/api/gax/retrying/RetryAlgorithm.java#L264

[alicejli]

Following the same format as googleapis/sdk-platform-java#2100:

cc: @blakeli0, @lqiu96 - let me know if you agree with the below proposals and I can implement the decision.

The main caveat to all of the below is if we plan to do a refactoring of how Retries work. If so, then I might suggest we not change the @BetaApi annotation.

ScheduledRetryingExecutor.createFuture

How long has this API been around?

6 years

Originally added in: googleapis/gax-java#590

How much variation occurred in the API over time?

Has not changed since it was introduced.

Any other context?

The original PR suggests this class was added for OpenCensus integration (which is now deprecated). They mention that when gax drops support for Java 7, it could be refactored. We have dropped support for Java 7 in gax, but any refactoring efforts would need to be part of a future planned project.

Proposal

The class it returns: CallbackChainRetryingFuture<> is marked as For internal use only, so we should add the @InternalApi to that class.

I think it would make sense to add @InternalExtensionOnly to this class(ScheduledRetryingExecutor.createFuture) since we don't want customers to change how it works.

RetrySettings.setLogicalTimeout

How long has this API been around?

3 years

Originally added in: googleapis/gax-java#1334

How much variation occurred in the API over time?

Has not changed since it was introduced.

Proposal

Remove the @BetaApi annotation
Add @InternalExtensionOnly annotation

RetryingContext

How long has this API been around?

6 years

Originally added in: googleapis/gax-java#590

How much variation occurred in the API over time?

It changed once, 3 years ago, to enable support for RetrySettings (googleapis/gax-java#1238)

Any other context?

The original PR suggests this class was added for OpenCensus integration (which is now deprecated), to access per-operation state. This is likely being used for OpenTelemetry (replacing OpenCensus).

Proposal

Remove the @BetaApi annotation.
Add @InternalExtensionOnly annotation

RetryAlgorithm.getResultAlgorithm

How long has this API been around?

6 years

Originally added in: googleapis/gax-java#632

How much variation occurred in the API over time?

It changed once, 3 years ago, to enable support for RetrySettings (googleapis/gax-java#1238)

Any other context?

The original PR suggests this class was added for OpenCensus integration (which is now deprecated), and the BetaAnnotation was because was for that future OpenCensus integration.

Proposal

Remove the @BetaApi annotation.
Add @InternalExtensionOnly annotation

RetryAlgorithm.getTimedAlgorithm

How long has this API been around?

6 years

Originally added in: googleapis/gax-java#632

How much variation occurred in the API over time?

It changed once, 3 years ago, to enable support for RetrySettings (googleapis/gax-java#1238)

Any other context?

The original PR suggests this class was added for OpenCensus integration (which is now deprecated), and the BetaAnnotation was because was for that future OpenCensus integration.

Proposal

Remove the @BetaApi annotation.
Add @InternalExtensionOnly annotation

[lqiu96]

From the Javadocs of @InternalExtensionOnly: https://github.com/googleapis/sdk-platform-java/blob/739ddbbbc662f43e46b9f420bbae685de9b3dbc6/api-common-java/src/main/java/com/google/api/core/InternalExtensionOnly.java#L53

I think this might require a breaking change?

From a quick glance, I think everything besides RetryingContext can just lose the @BetaApi annotation. Do we need to add the @InternalExtensionOnly annotation for it (might be missing a valid reason for it)?

[alicejli]

From the Javadocs of @InternalExtensionOnly:

https://github.com/googleapis/sdk-platform-java/blob/739ddbbbc662f43e46b9f420bbae685de9b3dbc6/api-common-java/src/main/java/com/google/api/core/InternalExtensionOnly.java#L53

I think this might require a breaking change?

From a quick glance, I think everything besides RetryingContext can just lose the @BetaApi annotation. Do we need to add the @InternalExtensionOnly annotation for it (might be missing a valid reason for it)?

Hmm good catch - I think most of gax in generally is probably ideally under a "Internal" type of flag as they aren't really things we want customers messing around with, but to your point sounds like we probably can't do that until we have a planned major version bump.

[lqiu96]

Yeah agreed. I think most (probably all) of the classes above should be marked with @InternalApi or @InternalExtensionOnly. Since adding both of those tags are API-breaking, I believe we'll have to save that for a major version bump. Additionally, I think we should add those tags to the class/ interface instead of the individual methods when that major version bump is scheduled.

For this ticket, I think we can just try to remove as much of the @BetaApi annotations where it makes sense. I see there are some comments about The surface for passing per operation state is not yet stable and I'm not entirely sure what that means. We should probably see if there are some internal docs about this and try to make that decision with more info.