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.

Sign up for GitHub

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

Jump to bottom

Allow configuration of the queued byte threshold at which a Stream is considered not ready #10977

Merged
merged 10 commits into from
Mar 21, 2024
Merged

Allow configuration of the queued byte threshold at which a Stream is considered not ready #10977

merged 10 commits into from
Mar 21, 2024

Conversation

jduo
Copy link
Contributor

@jduo jduo commented Mar 2, 2024

  • on clients this is exposed by setting a CallOption
  • on servers this is configured by calling a method on ServerCall or ServerStreamListener

@jduo jduo marked this pull request as draft March 2, 2024 00:45
@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Mar 2, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@jduo
Copy link
Contributor Author

jduo commented Mar 2, 2024

This is a draft to demonstrate the idea discussed in #5433 @ejona86:

  • It's a bit hard to tell what implementations of Stream and ServerCall need overrides for setOnReadyThreshold(). Some guidance here would be helpful
  • I'll work on adding unit tests and fixing up the documentation. If there are pointers on where to write the tests that'd be great.

@jduo jduo mentioned this pull request Mar 2, 2024
Closed
ejona86
ejona86 reviewed Mar 4, 2024
View reviewed changes
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.

Quick drive-by review. I think client-side will clean up nicely by my comment. Still need to look at server-side more.

core/src/main/java/io/grpc/internal/AbstractStream.java Outdated Show resolved Hide resolved
netty/src/main/java/io/grpc/netty/NettyClientStream.java Outdated Show resolved Hide resolved
api/src/main/java/io/grpc/CallOptions.java Show resolved Hide resolved
@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Mar 4, 2024
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Mar 4, 2024
@jduo jduo force-pushed the java-configurable-ready-threshold branch from 34b0da1 to 959473b Compare March 5, 2024 19:33
@jduo jduo marked this pull request as ready for review March 5, 2024 19:40
@jduo
Copy link
Contributor Author

jduo commented Mar 8, 2024

Hey @ejona86 , any more feedback? Particularly about the server side.

@jduo
Copy link
Contributor Author

jduo commented Mar 13, 2024

Hi @ejona86 , checking in on if there's an update.

I'm hoping to get this in March, the earlier the better. The Arrow project has a 3-month release cycle and there's a code freeze coming up at the end of March, and I'll be away for a from March 23rd until April.

Thanks!

@ejona86
Copy link
Member

ejona86 commented Mar 15, 2024

gRPC's next release is April 2nd, so the release cycles may not align for you ☹️. Even if we shifted that a few days, it sounds like that might not be enough for you.

@jduo
Copy link
Contributor Author

jduo commented Mar 15, 2024

gRPC's next release is April 2nd, so the release cycles may not align for you ☹️. Even if we shifted that a few days, it sounds like that might not be enough for you.

Thanks for the update. Yeah more realistically this will get into Arrow 17. We can still have Arrow Flight server developers use this before it officially makes it into Arrow though. Server developers can create the gRPC server themselves, configure this option, then apply the Arrow Flight stub.

@jduo jduo force-pushed the java-configurable-ready-threshold branch 2 times, most recently from 253fae4 to 75816e0 Compare March 16, 2024 13:50
@jduo
Copy link
Contributor Author

jduo commented Mar 16, 2024

I've added server tests now. I don't have tests for the TransmitStatusRuntimeExceptionInterceptor since I wasn't really sure how to use it. Hopefully this covers everything @ejona86

@jduo jduo force-pushed the java-configurable-ready-threshold branch 4 times, most recently from eb95ab5 to 98e228a Compare March 17, 2024 11:30
@jduo
Allow the queued byte threshold for a Stream to be ready to be config…
9024da0 
…urable

- on clients this is exposed by setting a CallOption
- on servers this is configured by calling a method on ServerCall or ServerStreamListener
@jduo jduo force-pushed the java-configurable-ready-threshold branch from 98e228a to 9024da0 Compare March 17, 2024 11:51
@sergiitk sergiitk mentioned this pull request Mar 18, 2024
Open
@jduo
Copy link
Contributor Author

jduo commented Mar 20, 2024

Hi @ejona86 , just wondering if you had a chance to look. I'll be away at the end of the week until the 3rd so it'd be great if we could get this wrapped up. Maybe this could get into 1.63 if it all looks OK.

ejona86
ejona86 reviewed Mar 20, 2024
View reviewed changes
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.

Please make your review changes as additional commits. When you squash the commits together during review it means I have to re-review the entire thing instead of only looking at your changes, which delays the review. We will squash the commits before merging to master.

api/src/main/java/io/grpc/PartialForwardingServerCall.java Outdated Show resolved Hide resolved
api/src/main/java/io/grpc/ServerCall.java Outdated Show resolved Hide resolved
api/src/main/java/io/grpc/CallOptions.java Show resolved Hide resolved
api/src/main/java/io/grpc/CallOptions.java Outdated Show resolved Hide resolved
api/src/main/java/io/grpc/ServerCall.java Outdated Show resolved Hide resolved
core/src/main/java/io/grpc/internal/AbstractServerStream.java Outdated Show resolved Hide resolved
util/src/main/java/io/grpc/util/TransmitStatusRuntimeExceptionInterceptor.java Outdated Show resolved Hide resolved
stub/src/main/java/io/grpc/stub/ServerCallStreamObserver.java Outdated Show resolved Hide resolved
okhttp/src/main/java/io/grpc/okhttp/OkHttpClientStream.java Outdated Show resolved Hide resolved
inprocess/src/main/java/io/grpc/inprocess/InProcessTransport.java Outdated Show resolved Hide resolved
@ejona86
Copy link
Member

ejona86 commented Mar 21, 2024

I'll be away at the end of the week until the 3rd

At this point is this "I'll be gone tomorrow"? If so (if you have no time), I'd accept this with the threshold checks in OkHttpClientStream and the like. But we do need to get some of the other easier things resolved.

@jduo
Copy link
Contributor Author

jduo commented Mar 21, 2024

I'll be away at the end of the week until the 3rd

At this point is this "I'll be gone tomorrow"? If so (if you have no time), I'd accept this with the threshold checks in OkHttpClientStream and the like. But we do need to get some of the other easier things resolved.

I've covered the javadoc-related ones mostly now. If you can give some more guidance around the AbstractClientStream constructor I can pick that up too.

Thanks!

@jduo
PR feedback 3 part 2
7cf43d2 
* Change setting on client to happen during TransportState constructor
* Fix checkstyle issues
@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Mar 21, 2024
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Mar 21, 2024
@ejona86 ejona86 requested a review from larry-safran March 21, 2024 14:35
@jduo
Copy link
Contributor Author

jduo commented Mar 21, 2024

The failure in JDK 8 testing looks unrelated. Is it flakey?
io.grpc.servlet.jakarta.UndertowInteropTest > pingPong FAILED java.lang.AssertionError at Assert.java:89

@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Mar 21, 2024
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Mar 21, 2024
@ejona86
Copy link
Member

ejona86 commented Mar 21, 2024

Yeah, UndertowInteropTest.pingPong is likely a flake. I've restarted it, but I wouldn't be concerned. We can still merge if it fails.

@jduo
Copy link
Contributor Author

jduo commented Mar 21, 2024

Looks like there are some Android classes that need to be updated (SingleMessageServerStream and MultiMessageServerStream).

These seem to use a different class hierarchy than using a Transport so I'm not sure what the implementation entails. For now I'm just going to make setOnReadyThreshold() a no-op.

Unfortunately my Android Studio installation isn't working right so I'm having difficulty seeing Android build issues locally and am only spotting them through Kokoro.

@ejona86
Copy link
Member

ejona86 commented Mar 21, 2024 •
edited
Loading

No-op is fine. Since the Android build requires us to add the "kokoro:run" label for each change, it might make sense for us to fix it up instead of you. Do you have a change there in-flight, or is it safe to push to your branch?

@jduo
Fix Android build
03d81e7 
Implementations of ServerStream
@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Mar 21, 2024
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Mar 21, 2024
@jduo
Copy link
Contributor Author

jduo commented Mar 21, 2024

No-op is fine. Since the Android build requires us to add the "kokoro:run" label for each change, it might make sense for us to fix it up instead of you. Do you have a change there in-flight, or is it safe to push to your branch?

I added no-ops for the two ServerStream implementations in binder. No other changes in-flight.
It'd be great if you could finish up the Android build.

@ejona86
Copy link
Member

ejona86 commented Mar 21, 2024

I just built your "Fix Android build" commit locally and it worked fine. So the Android CI should turn green this time.

@larry-safran larry-safran changed the title Allow the queued byte threshold for a Stream to be ready to be configurable Allow configuration of the queued byte threshold at which a Stream is considered not ready Mar 21, 2024
@larry-safran larry-safran merged commit 2c83ef0 into grpc:master Mar 21, 2024
15 checks passed
@ejona86
Copy link
Member

ejona86 commented Mar 22, 2024

@jduo, thank you! This made it in before the 1.63 branch cut, so it will be in the next release. (The branch cut was scheduled for Tuesday, but it had been forgotten about a bit so hasn't been done yet.)

@jduo
Copy link
Contributor Author

jduo commented Mar 22, 2024

@jduo, thank you! This made it in before the 1.63 branch cut, so it will be in the next release. (The branch cut was scheduled for Tuesday, but it had been forgotten about a bit so hasn't been done yet.)

Thanks for your help getting this in for our timeline @ejona86 @larry-safran !

@ran-su ran-su mentioned this pull request Mar 27, 2024
Merged
@ejona86 ejona86 mentioned this pull request Mar 29, 2024
Merged
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ejona86 ejona86 ejona86 approved these changes

@larry-safran larry-safran Awaiting requested review from larry-safran

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jduo @ejona86 @grpc-kokoro @larry-safran