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

stub: try enable GRPC_CLIENT_CALL_REJECT_RUNNABLE #9035

Merged
merged 1 commit into from
Apr 1, 2022

Conversation

YifeiZhuang
Copy link
Contributor

Did dryrun tests on cl/435778399, enabling the flag, for multiple times, not seeing failures cases that are obviously caused by our change, so I am willing to give it a try. @ejona86 Wdyt?

Tests tended to be flaky. But the latest TAP train test signals that google3 might be ok if we enable it:

@YifeiZhuang YifeiZhuang requested a review from ejona86 March 30, 2022 22:37
Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests look good. Although we still need to fix the cases in ClientCalls.blockingUnaryCall() where we shutdown the executor without waiting for the RPC closure.

@YifeiZhuang
Copy link
Contributor Author

YifeiZhuang commented Mar 31, 2022

The tests look good. Although we still need to fix the cases in ClientCalls.blockingUnaryCall() where we shutdown the executor without waiting for the RPC closure.

you mean the incremental fix that we only shutdown the executor when RPC is successful (status is ok)?
we were talking about failure cases that we shutdown RPC without knowing if they are closed.
On those failures cases I assume we should not keep executing runnables that is added to ThreadlessExecutor.execute() ?
I think we can merge this change first? It is more strict so more risk, but would be easier to rollback.

@YifeiZhuang
Copy link
Contributor Author

we discussed this. Only enable when ThreadlessExecutor.shutdown guarantees that is happens after call is closed. shutdown is changed in #9042

Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming #9041 or #9042 goes in before this, this LGTM

@YifeiZhuang YifeiZhuang merged commit c53c3ad into grpc:master Apr 1, 2022
@YifeiZhuang YifeiZhuang deleted the enable-rejction branch April 1, 2022 15:51
YifeiZhuang added a commit to YifeiZhuang/grpc-java that referenced this pull request Apr 6, 2022
YifeiZhuang added a commit that referenced this pull request Apr 6, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants