You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
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.
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
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
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
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)?
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.
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.
Activity
alicejli commentedon Mar 27, 2024
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
@BetaApiannotation.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@InternalApito that class.I think it would make sense to add
@InternalExtensionOnlyto 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
@BetaApiannotationAdd
@InternalExtensionOnlyannotationRetryingContext
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
@BetaApiannotation.Add
@InternalExtensionOnlyannotationRetryAlgorithm.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
@BetaApiannotation.Add
@InternalExtensionOnlyannotationRetryAlgorithm.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
@BetaApiannotation.Add
@InternalExtensionOnlyannotationlqiu96 commentedon Mar 27, 2024
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#L53I think this might require a breaking change?
From a quick glance, I think everything besides
RetryingContextcan just lose the@BetaApiannotation. Do we need to add the@InternalExtensionOnlyannotation for it (might be missing a valid reason for it)?alicejli commentedon Mar 28, 2024
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 commentedon Mar 28, 2024
Yeah agreed. I think most (probably all) of the classes above should be marked with
@InternalApior@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
@BetaApiannotations where it makes sense. I see there are some comments aboutThe surface for passing per operation state is not yet stableand 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.[-]gax: Consider removing @BetaApi and/or adding @InternalExtensionOnly to retry surface[/-][+][sdk-platform-java] gax: Consider removing @BetaApi and/or adding @InternalExtensionOnly to retry surface[/+]