Skip to content
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

Tracking Issue for (Call|ServerCall)StreamObserver being Experimental. #1788

Closed
carl-mastrangelo opened this issue May 3, 2016 · 13 comments
Closed
Labels
experimental API Issue tracks stabilizing an experimental API
Milestone

Comments

@carl-mastrangelo
Copy link
Contributor

Specific usages:

  • CallStreamObserver
  • ServerCallStreamObserver
@carl-mastrangelo carl-mastrangelo added this to the Unscheduled milestone May 3, 2016
@dapengzhang0 dapengzhang0 reopened this May 20, 2016
@ejona86 ejona86 added the experimental API Issue tracks stabilizing an experimental API label Aug 22, 2016
@wu-sheng
Copy link

The ClientCallStreamObserver and CallStreamObserver have be annotated by @ExperimentalApi. Can you tell me why?

Is the api good enough to use on product env. ? This API is very useful for our project. But I need grpc-team to confirm.

Thanks. @ejona86

@ejona86
Copy link
Member

ejona86 commented Nov 16, 2016

The APIs are functional but we still reserve the right to change the API. Once there have been some users and agreement that the APIs are working well then we would remove the annotation.

I'm not quite settled with disableAutoInboundFlowControl, for example, because it still has an implicit request() when the RPC starts. The basic functionally it provides is likely to stay (since flow control is essential) but the precise form can change.

@vkedia
Copy link

vkedia commented May 22, 2017

@ejona86 Do you have any further updates on making this non experimental? We use this API in the cloud spanner client library and it has worked well for us so far. But this being experimental prevents our client from going GA.

@ejona86 ejona86 modified the milestones: Next, Unscheduled May 22, 2017
@ejona86
Copy link
Member

ejona86 commented May 22, 2017

@vkedia, no I don't. This depends on stabilizing ClientCallStreamObserver and I think there are some issues that need to be resolved for client vs server APIs.

@ejona86
Copy link
Member

ejona86 commented Nov 2, 2017

API discussion notes for disableAutoInboundFlowControl:
General agreement that a different name would make the API easier to understand and worth converting existing users. Although not important enough by itself, removing the special case on unary would be good. Since we're wanting a new name, also remove unary special case.

Names:
disableAutoInboundFlowControl
disableAutoRequest +1 +1
disableImplicitRequest
useExplicitRequest
setAutoRequest(false) +1 +1

I'm interested in eventually adding the auto/implicit request() calls to ClientCall/ServerCall (but it would be opt-in). That influenced people's preference for name (e.g., moved a vote from useExplicitRequest to setAutoRequest)
useImplicitRequest
enableAutoRequest
enableImplicitRequest
setAutoRequest(true)

No clear winner. Will loop back around to it.

@carl-mastrangelo
Copy link
Contributor Author

requestOnMessage?

barnardb added a commit to barnardb/grpc-java that referenced this issue Mar 14, 2018
This commit prevents ClientCalls from making an initial request for
response messages when the user provides a ClientResponseObserver that
has disabled auto inbound flow control by calling
ClientCallStreamObserver.disableAutoInboundFlowControl() in beforeStart.

The previous behaviour was confusing and undersirable for observers
trying to implement their own flow control,
as values were produced before they were requested.
This behaviour was mentioned in grpc#1788#issuecomment-260898948.

The initial requests are still made for ResponseObservers that haven't
disabled auto inbound flow control.
barnardb added a commit to barnardb/grpc-java that referenced this issue Mar 14, 2018
This commit prevents ClientCalls from making an initial request for
response messages when the user provides a ClientResponseObserver that
has disabled auto inbound flow control by calling
ClientCallStreamObserver.disableAutoInboundFlowControl() in beforeStart.

The previous behaviour was confusing and undersirable for observers
trying to implement their own flow control,
as values were produced before they were requested.
This behaviour was mentioned in
grpc#1788 (comment)

The initial requests are still made for ResponseObservers that haven't
disabled auto inbound flow control.
barnardb added a commit to barnardb/grpc-java that referenced this issue Mar 14, 2018
This commit prevents ClientCalls from making an initial request for
response messages when the user provides a ClientResponseObserver that
has disabled auto inbound flow control by calling
ClientCallStreamObserver.disableAutoInboundFlowControl() in beforeStart.

The previous behaviour was confusing and undersirable for observers
trying to implement their own flow control,
as values were produced before they were requested.
This behaviour was mentioned in
grpc#1788 (comment)

The initial requests are still made for ResponseObservers that haven't
disabled auto inbound flow control.
@ejona86
Copy link
Member

ejona86 commented Mar 22, 2018

API discussion notes:
I "randomly" selected disableAutoRequest as the winner. If in the future we add a similar API to ClientCall/ServerCall it would be named enableAutoRequest.

@carl-mastrangelo
Copy link
Contributor Author

Held:

  • ClientCallStreamObserver and ServerCallStreamObserverr will be stabilized, and have methods from CallStreamObserver forwarded . (pending disableAutoRequest being added)
  • CallStreamObserver will remain experimental
  • ClientResponseObserver will be stabilized, due to needing it for disableAutoRequest and setOnReadyHandler
  • We agree ClientResponseObserver is a non-discoverable, forgettable name, we have no better alternative though. This is unfortunate, but we will live with it.

@carl-mastrangelo
Copy link
Contributor Author

@ejona86 does disableAutoRequest() apply to Unary calls as well? I can't remember.

@ejona86
Copy link
Member

ejona86 commented Feb 13, 2019

@carl-mastrangelo, we would not explicitly make it behave differently for unary (like the current API). Some parts may break down due to other parts of the API. For example, unary for servers will receive the request (as an argument) before having an opportunity to call disableAutoRequest() first.

@ejona86
Copy link
Member

ejona86 commented May 14, 2020

#6807 added disableAutoRequestWithInitial. We made decisions on that not considering server-side.

api-review decision:

  • We want to use disableAutoRequest on server-side and disableAutoRequestWithInitial on client-side
  • We chose that in favor of having disableAutoRequest on both client and server, with disableAutoRequestWithInitial only on client-side. Weren't really against this option, but anyone with an opinion liked the other one slightly more and the other option does not prevent this option in the future (we can still add disableAutoRequest to CallStreamObserver in the future)

Adding release blocker just so it doesn't slip through the cracks for this release, but we wouldn't actually delay the release for it.

@ejona86 ejona86 removed the TODO:release blocker Issue/PR is important enough to delay the release. Removed after release issues resolved label May 29, 2020
@ghost
Copy link

ghost commented Jun 3, 2020

Is this feature still experimental? 1.29.1 source code still claims this to be experimental. We are only now beginning to use gRPC in our project and want to know if this feature is recommended or not.

@ejona86
Copy link
Member

ejona86 commented Jun 3, 2020

This feature is still experimental. It is "production ready" but the precise API is seeing changes. It is now rapidly approaching stabilization. In v1.30 (being released today) disableAutoInboundFlowControl is being replaced with disableAutoRequest (both APIs available simultaneously). I expect some minor tweaks to edge cases for disableAutoRequest, things that wouldn't impact most people, in v1.31. But we may stabilize the important parts of the API in v1.31. In our typical fashion, we'll leave the old API in-place for a while to ease migration.

@creamsoup creamsoup removed their assignment Jul 8, 2020
@dapengzhang0 dapengzhang0 modified the milestones: Next, 1.37 Mar 10, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 9, 2021
benjaminp added a commit to benjaminp/grpc-java that referenced this issue Sep 7, 2021
I believe this was overlooked in grpc#7938's work to fix grpc#1788.
benjaminp added a commit to benjaminp/grpc-java that referenced this issue Sep 7, 2021
I believe this was overlooked in grpc#7938's work to fix grpc#1788.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
experimental API Issue tracks stabilizing an experimental API
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants