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

Use RESOURCE_EXHAUSTED for max message size limits. #10612

Merged
merged 3 commits into from
Apr 13, 2017

Conversation

markdroth
Copy link
Member

CC @carl-mastrangelo @lyuxuan to make sure that the Java and Go implementations use the same code.

@grpc-kokoro
Copy link

No significant performance differences

@grpc-kokoro
Copy link

Performance differences noted:
Benchmark                                      cpu_time    real_time
-------------------------------------------  ----------  -----------
BM_AcquireMutex                                   -5.50        -5.50
BM_CallCreateDestroy<InsecureChannel>           -492.50      -492.50
BM_ClosureSched3OnExecCtx                        -27.50       -27.50
BM_CreateDestroyPollset                           +8.50        +8.50
BM_EmptyCore                                    -379.50      -379.50
BM_ErrorCreateAndSetIntAndStr                   +286.50      +286.50
BM_ErrorCreateAndSetIntLoop                       +9.00        +9.00
BM_ErrorCreateAndSetStrLoop                      +27.00       +27.00
BM_ErrorGetStatusCode<ErrorWithHttpError>         -6.50        -6.50
BM_ErrorHttpError<ErrorNone>                     +21.50       +21.50
BM_ErrorStringOnNewError<SimpleError>          +1726.00     +1726.00
BM_HasClearGrpcStatus<ErrorNone>                  +7.00        +7.00
BM_MetadataRefUnrefInterned                       +7.00        +7.00
BM_Pass1Core                                     -59.50       -59.50
BM_Pluck1Core                                    -59.50       -59.50
BM_PollEmptyPollset_SpeedOfLight/1/10             +8.50        +8.50
BM_PollEmptyPollset_SpeedOfLight/1/100           +11.00       +11.00
BM_PollEmptyPollset_SpeedOfLight/1/1000          +11.00       +11.50
BM_PollEmptyPollset_SpeedOfLight/1/9.76562k      +15.50       +15.50
BM_TryAcquireMutex                               -10.50       -10.50

@carl-mastrangelo
Copy link
Contributor

@markdroth thanks for the heads up, Java will use this code starting in 1.4.

@markdroth
Copy link
Member Author

Known failures: #9542 #10576 #10582 #10481 #10643

@markdroth markdroth merged commit 023c982 into grpc:master Apr 13, 2017
@markdroth markdroth deleted the max_message_size_status_code branch April 13, 2017 21:52
@therc
Copy link

therc commented Jul 27, 2017

This was an unfortunate choice. https://github.com/grpc/grpc/blob/master/doc/statuscodes.md defines RESOURCE_EXHAUSTED as being generated by both the server (full queue, out of quota, etc.) or the client (response too large). The problem with that is that, most of the time, it's OK and desirable to retry the former case, while it makes no sense in the latter. Unless responses vary between identical calls, if a message is too large the first time, it'll be too large on every retry, too. And because we're talking about >4MB in the default case, this will result each time in quite a waste of cycles, memory and bandwidth.
I suppose it is too late now for any changes. Is it recommended for clients that care simply to hard code a check against the error string that is used by the gRPC implementation they rely on?

@markdroth
Copy link
Member Author

I'll let @a11r respond to this.

Ultimately, I think this is an unfortunate consequence of having a fixed set of response codes, especially given that the same set of codes is returned by both gRPC and the application. I would not recommend that applications depend on the human-readable text strings, because there is no guarantee that those won't change over time. But I don't have a better solution to recommend.

@therc
Copy link

therc commented Jul 27, 2017

Yes, the text string check is a very desperate last resort, in absence of a better mechanism. We're adding an opt-in to disable retries for "message too large" errors (to accomodate both the case of an hypothetical GetQueueContents() and GetQueueItem(): perhaps the client is OK with retrying the former) and it'll be treated as an optimisation. I'll have to find a way to make it work with service configs, of course.

We'll also have a test to make sure to catch text changes over time, or at least the trivial ones.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants