Allow configuration of the queued byte threshold at which a Stream is considered not ready by jduo · Pull Request #10977 · grpc/grpc-java

jduo · 2024-03-02T00:45:49Z

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

linux-foundation-easycla · 2024-03-02T00:45:55Z

The committers listed above are authorized under a signed CLA.

✅ login: jduo / name: James Duong (1a6b599, 9024da0, 23bdd1f, 03d81e7, 63a454d, ea79d99, 673e2ce, 7cf43d2, f0cf726, 3c78fe0)

jduo · 2024-03-02T00:49:57Z

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.

ejona86

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

jduo · 2024-03-08T19:17:14Z

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

jduo · 2024-03-13T13:13:36Z

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 · 2024-03-15T15:26:30Z

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 · 2024-03-15T16:36:46Z

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 · 2024-03-16T13:52:01Z

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

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

jduo · 2024-03-20T18:54:51Z

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

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.

ejona86 · 2024-03-21T00:28:40Z

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 · 2024-03-21T00:33:17Z

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!

* Change setting on client to happen during TransportState constructor * Fix checkstyle issues

jduo · 2024-03-21T19:04:39Z

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 · 2024-03-21T20:30:02Z

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 · 2024-03-21T21:27:58Z

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 · 2024-03-21T21:29:58Z

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?

Implementations of ServerStream

jduo · 2024-03-21T21:34:45Z

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 · 2024-03-21T21:35:23Z

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

ejona86 · 2024-03-22T00:07:28Z

@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 · 2024-03-22T03:22:05Z

@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 !

jduo marked this pull request as draft March 2, 2024 00:45

jduo mentioned this pull request Mar 2, 2024

Allow configuring onReady threshold #5433

Closed

ejona86 reviewed Mar 4, 2024

View reviewed changes

Comment thread core/src/main/java/io/grpc/internal/AbstractStream.java

Comment thread netty/src/main/java/io/grpc/netty/NettyClientStream.java Outdated

Comment thread api/src/main/java/io/grpc/CallOptions.java

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 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 force-pushed the java-configurable-ready-threshold branch from 34b0da1 to 959473b Compare March 5, 2024 19:33

jduo marked this pull request as ready for review March 5, 2024 19:40

ejona86 reviewed Mar 15, 2024

View reviewed changes

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

jduo force-pushed the java-configurable-ready-threshold branch 4 times, most recently from eb95ab5 to 98e228a Compare March 17, 2024 11:30

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 force-pushed the java-configurable-ready-threshold branch from 98e228a to 9024da0 Compare March 17, 2024 11:51

sergiitk mentioned this pull request Mar 18, 2024

Tracking issue for setOnReadyThreshold public API #11021

Open

ejona86 reviewed Mar 20, 2024

View reviewed changes

PR feedback round 3

23bdd1f

PR feedback 3 part 2

7cf43d2

* Change setting on client to happen during TransportState constructor * Fix checkstyle issues

jduo added 3 commits March 20, 2024 20:56

Merge branch 'master' into java-configurable-ready-threshold

63a454d

Add test for supplying CallOptions to transport state

f0cf726

checkstyle

ea79d99

ejona86 approved these changes Mar 21, 2024

View reviewed changes

Comment thread core/src/main/java/io/grpc/internal/ServerCallImpl.java Outdated

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 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 requested a review from larry-safran March 21, 2024 14:35

jduo added 3 commits March 21, 2024 11:20

Fix Android build

1a6b599

Remove @experimentalapi from classes in internal package

673e2ce

checkstyle

3c78fe0

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 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

Fix Android build

03d81e7

Implementations of ServerStream

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 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

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 merged commit 2c83ef0 into grpc:master Mar 21, 2024

ran-su mentioned this pull request Mar 27, 2024

Make setOnReadyThreshold() a noop method instead of abstract. #11044

Merged

ejona86 mentioned this pull request Mar 29, 2024

Make setOnReadyThreshold() a noop method instead of abstract. (1.63.x backport) #11059

Merged

github-actions Bot locked as resolved and limited conversation to collaborators Jun 20, 2024

Uh oh!

Conversation

jduo commented Mar 2, 2024

Uh oh!

linux-foundation-easycla Bot commented Mar 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jduo commented Mar 2, 2024

Uh oh!

ejona86 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jduo commented Mar 8, 2024

Uh oh!

jduo commented Mar 13, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ejona86 commented Mar 15, 2024

Uh oh!

jduo commented Mar 15, 2024

Uh oh!

jduo commented Mar 16, 2024

Uh oh!

jduo commented Mar 20, 2024

Uh oh!

ejona86 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ejona86 commented Mar 21, 2024

Uh oh!

jduo commented Mar 21, 2024

Uh oh!

Uh oh!

jduo commented Mar 21, 2024

Uh oh!

ejona86 commented Mar 21, 2024

Uh oh!

jduo commented Mar 21, 2024

Uh oh!

ejona86 commented Mar 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jduo commented Mar 21, 2024

Uh oh!

ejona86 commented Mar 21, 2024

Uh oh!

ejona86 commented Mar 22, 2024

Uh oh!

jduo commented Mar 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

linux-foundation-easycla Bot commented Mar 2, 2024 •

edited

Loading

ejona86 commented Mar 21, 2024 •

edited

Loading