Page MenuHomeElementl

[RFC][grpc] turn on gzip and set higher receive limit
ClosedPublic

Authored by alangenfeld on Jul 27 2021, 7:44 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, May 28, 9:58 AM
Unknown Object (File)
Thu, May 18, 11:29 PM
Unknown Object (File)
Sat, May 13, 12:41 PM
Unknown Object (File)
Fri, May 12, 5:18 AM
Unknown Object (File)
Thu, May 11, 9:51 AM
Unknown Object (File)
Wed, May 10, 10:40 AM
Unknown Object (File)
Apr 28 2023, 12:49 PM
Unknown Object (File)
Mar 26 2023, 10:24 PM
Subscribers
None

Details

Summary

The GRPC message limit is designed to prevent a client consuming unexpected memory by being pushed a large response by the server.

Due to this goal, the compression does not save us, as the limit is applied on the decompressed message.

It is still useful to enable compression as our payloads are currently filled with redundant structures that gzip eats for breakfast. The example from the test plan screen shot shows a compression down to 3% of the original message size.

Previously we have worked around this limit by creating streaming versions of the endpoints, but then we just concat all the slices together anyway so why not just raise the limit and receive the whole payload directly.

Test Plan

integration

using dagit -f python_modules/dagit/dagit_tests/stress/megadags.py verify that message limits previously hit are no longer hit

use wireshark to inspect local traffic between

dagster api grpc -f python_modules/dagit/dagit_tests/stress/megadags.py -p 5000

and

dagit --grpc-port 5000

Screen Shot 2021-07-27 at 3.13.22 PM.png (283×817 px, 71 KB)

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

alangenfeld retitled this revision from [RFC][grpc] set higher receive limit to [RFC][grpc] turn on gzip and set higher receive limit.Jul 27 2021, 8:25 PM
alangenfeld edited the summary of this revision. (Show Details)
alangenfeld edited the test plan for this revision. (Show Details)

it appears compression does not kick on our streaming variants - so if we go this direction we may want to move away from those as well

Screen Shot 2021-07-27 at 3.32.44 PM.png (412×809 px, 64 KB)

this looks good, but is back-compat a concern? if you upgrade dagit and don't upgrade the server will things break?

I have an internal test you could run to check that we should really port into OSS...

python_modules/dagster/dagster/grpc/client.py
89

should we allow users to set this on the instance or an env var as an escape hatch?

this looks good, but is back-compat a concern? if you upgrade dagit and don't upgrade the server will things break?

I can double check - but from my testing of seeing if setting it just on the client or just on the server - it just degrades in to not compressing, no errors or anything.

python_modules/dagster/dagster/grpc/client.py
89

yea thats a good idea

This revision is now accepted and ready to land.Jul 27 2021, 9:37 PM

add env, test via 'DAGSTER_GRPC_MAX_RX_BYTES=5000000 dagit --grpc-port 5000'