OkHttp: Flushes headers out immediately for client streaming and bidi streaming #274

madongfly · 2015-04-07T07:03:06Z

Flushes headers out immediately for OkHttp transport, so that the server can be aware of the stream before receiving real data (or half close).

Adds corresponding test to AbstractTransportTest.

iamqizhao · 2015-04-07T07:09:54Z

I am curious why this is the way to do. This actually doubles the number of
syscall on client writing path because otherwise header and data can be
fused into a single write syscall (unless the header has some
user-specified metadata). This has some performance impact given the proper
designed benchmarks.

On Tue, Apr 7, 2015 at 12:03 AM, Xudong Ma notifications@github.com wrote:

Flushes headers out immediately for OkHttp transport, so that the server
can be aware of the stream before receiving real data (or half close).

Adds corresponding test to AbstractTransportTest.

You can view, comment on, or merge this pull request online at:

#274
Commit Summary

Fix for OkHttp may be missing flush after sending headers #257.

File Changes

M
integration-testing/src/main/java/io/grpc/testing/integration/AbstractTransportTest.java
https://github.com/grpc/grpc-java/pull/274/files#diff-0 (46)

M
integration-testing/src/main/java/io/grpc/testing/integration/TestServiceImpl.java
https://github.com/grpc/grpc-java/pull/274/files#diff-1 (33)

M
integration-testing/src/main/proto/io/grpc/testing/integration/test.proto
https://github.com/grpc/grpc-java/pull/274/files#diff-2 (5)

M
okhttp/src/main/java/io/grpc/transport/okhttp/OkHttpClientTransport.java
https://github.com/grpc/grpc-java/pull/274/files#diff-3 (2)

Patch Links:

https://github.com/grpc/grpc-java/pull/274.patch

https://github.com/grpc/grpc-java/pull/274.diff

—
Reply to this email directly or view it on GitHub
#274.

madongfly · 2015-04-07T07:24:24Z

@iamqizhao, you are right, this flush should only be made for client streaming and bi-di streaming. I'll continue working on it.

iamqizhao · 2015-04-07T07:29:29Z

Can you elaborate why it is necessary for client streaming and bi-di
streaming? Thanks.

On Tue, Apr 7, 2015 at 12:24 AM, Xudong Ma notifications@github.com wrote:

@iamqizhao https://github.com/iamqizhao, you are right, this flush
should only be made for client streaming and bi-di streaming. I'll continue
working on it.

—
Reply to this email directly or view it on GitHub
#274 (comment).

madongfly · 2015-04-07T07:38:26Z

@iamqizhao, my understanding is that there may be some use cases like I made in the test: the server side wants to decide (maybe after checking some metadata in the Header like you said) whether the client can start sending.

@ejona86 and @a11r can provide more info.

iamqizhao · 2015-04-07T07:43:29Z

On Tue, Apr 7, 2015 at 12:38 AM, Xudong Ma notifications@github.com wrote:

@iamqizhao https://github.com/iamqizhao, my understanding is that there
may be some use cases like I made in the test: the server side wants to
decide (maybe after checking some metadata in the Header like you said)
whether the client can start sending.

Yup, but this is a special treatment for header metadata. We should not
flush if that is not the case.

@ejona86 https://github.com/ejona86 and @a11r https://github.com/a11r
can provide more info.

—
Reply to this email directly or view it on GitHub
#274 (comment).

ejona86 · 2015-04-07T16:24:02Z

integration-testing/src/main/java/io/grpc/testing/integration/AbstractTransportTest.java

+    StreamingInputCallResponse response = responseObserver.firstValue().get();
+    // Server sent a special message to tell us start uploading.
+    assertEquals(-1, response.getAggregatedPayloadSize());
+    for (StreamingInputCallRequest request : requests) {


Is there much of a point in actually sending any requests at this point? Even if one request is useful, why 4?

No point, just copied the code from the clientStreaming test.

I'll change it to just 1.

ejona86 · 2015-04-07T16:54:15Z

Yup, but this is a special treatment for header metadata.

@iamqizhao, no. That is just the wanted semantics. I would be strongly oppose special casing the flush only if there is custom metadata. We should always do it or never do it. Given the zero-message semantics of streaming and the ability of long-lived requests, the most sensible thing seemed to be to flush. @a11r felt the same way, even before I shared my opinion.

madongfly · 2015-04-09T01:12:56Z

@ejona86 Could you take another look, thanks!

madongfly · 2015-04-10T07:43:34Z

@ejona86 PTAL, thanks!

iamqizhao · 2015-04-12T23:08:46Z

On Tue, Apr 7, 2015 at 9:54 AM, Eric Anderson notifications@github.com
wrote:

Yup, but this is a special treatment for header metadata.

@iamqizhao https://github.com/iamqizhao, no. That is just the wanted
semantics. I would be strongly oppose special casing the flush only if
there is custom metadata. We should always do it or never do it. Given the
zero-message semantics of streaming and the ability of long-lived requests,
the most sensible thing seemed to be to flush. @a11r
https://github.com/a11r felt the same way, even before I shared my
opinion.

It is an important performance optimization and has non-trivial performance
impact especially for high-qps applications. I will keep it as an important
knob for performance tuning. I will talk to abhishek sometime next week.

—
Reply to this email directly or view it on GitHub
#274 (comment).

madongfly · 2015-04-21T09:17:16Z

Ping, any update? @iamqizhao @ejona86

ejona86 · 2015-04-21T16:58:54Z

@madongfly, I'm still unsettled about the integration test. Can we do this change without a new integration test? If you'd like, you can add a new TODO to our integration test doc (if you reference me (with @) then I'd be happy to merge the change).

A unit test in OkHttpClientTransportTest should be enough for now.

madongfly · 2015-04-22T02:18:04Z

@ejona86 Why do you dislike a new integration test? Are you planing to make the integration test only implement the tests listed on the integration test doc?

Based on the only change made in this PR: flushing after synStream(), I think either we have a integration test to check the behavior in the wire level, or we just trust the newly added flush call and ignore the test. Having an unit test in OkHttpClientTransportTest to check whether the flush is called seems meaningless.

ejona86 · 2015-04-22T16:41:08Z

@madongfly, the integration test proto is being kept in sync between all the languages. It is shared. I don't want to add something to it that I don't think should be implemented in all languages in the current form.

I'm suggesting that integration test is not a strong option at the moment. If we have an integration test, then yes, you could argue that a unit test is meaningless (although I could disagree with that). But a unit test doesn't seem meaningless if we don't have an integration test.

Given the size and nature of the change, I'm not super-concerned about a unit test (I'd merge without), but it would be nice.

madongfly · 2015-04-23T05:38:01Z

@ejona86 Done, PTAL, thanks!

ejona86 · 2015-04-27T17:17:43Z

@madongfly, LGTM.

#338 has advanced enough that we know how it relates to this PR. I was delaying to see how the Netty version looked.

…are of the stream before receiving real data (or half close). Resolves grpc#257.

ejona86 reviewed Apr 7, 2015
View reviewed changes

madongfly changed the title ~~Fix for #257.~~ OkHttp: Flushes headers out immediately. Apr 8, 2015

madongfly changed the title ~~OkHttp: Flushes headers out immediately.~~ OkHttp: Flushes headers out immediately for client streaming and bidi streaming Apr 8, 2015

madongfly force-pushed the sendheader branch from ec0131e to 8c2875f Compare April 10, 2015 07:41

ejona86 mentioned this pull request Apr 23, 2015

Throughput: AbstractServerStream does unnecessary flushes #331

Closed

okhttp: Flushes headers out immediately, so that the server can be aw…

4b00476

…are of the stream before receiving real data (or half close). Resolves grpc#257.

madongfly force-pushed the sendheader branch from 589f53f to 4b00476 Compare April 27, 2015 20:33

madongfly merged commit 4b00476 into grpc:master Apr 27, 2015

madongfly deleted the sendheader branch April 27, 2015 21:01

lock bot locked as resolved and limited conversation to collaborators Jan 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OkHttp: Flushes headers out immediately for client streaming and bidi streaming #274

OkHttp: Flushes headers out immediately for client streaming and bidi streaming #274

madongfly commented Apr 7, 2015

iamqizhao commented Apr 7, 2015

Adds corresponding test to AbstractTransportTest.

madongfly commented Apr 7, 2015

iamqizhao commented Apr 7, 2015

madongfly commented Apr 7, 2015

iamqizhao commented Apr 7, 2015

ejona86 Apr 7, 2015

madongfly Apr 8, 2015

madongfly Apr 8, 2015

ejona86 commented Apr 7, 2015

madongfly commented Apr 9, 2015

madongfly commented Apr 10, 2015

iamqizhao commented Apr 12, 2015

madongfly commented Apr 21, 2015

ejona86 commented Apr 21, 2015

madongfly commented Apr 22, 2015

ejona86 commented Apr 22, 2015

madongfly commented Apr 23, 2015

ejona86 commented Apr 27, 2015

OkHttp: Flushes headers out immediately for client streaming and bidi streaming #274

OkHttp: Flushes headers out immediately for client streaming and bidi streaming #274

Conversation

madongfly commented Apr 7, 2015

iamqizhao commented Apr 7, 2015

Adds corresponding test to AbstractTransportTest.

madongfly commented Apr 7, 2015

iamqizhao commented Apr 7, 2015

madongfly commented Apr 7, 2015

iamqizhao commented Apr 7, 2015

ejona86 Apr 7, 2015

Choose a reason for hiding this comment

madongfly Apr 8, 2015

Choose a reason for hiding this comment

madongfly Apr 8, 2015

Choose a reason for hiding this comment

ejona86 commented Apr 7, 2015

madongfly commented Apr 9, 2015

madongfly commented Apr 10, 2015

iamqizhao commented Apr 12, 2015

madongfly commented Apr 21, 2015

ejona86 commented Apr 21, 2015

madongfly commented Apr 22, 2015

ejona86 commented Apr 22, 2015

madongfly commented Apr 23, 2015

ejona86 commented Apr 27, 2015