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

Add option to increase max message size #246

Merged
merged 6 commits into from
Sep 15, 2018
Merged

Add option to increase max message size #246

merged 6 commits into from
Sep 15, 2018

Conversation

nevi-me
Copy link
Contributor

@nevi-me nevi-me commented Sep 15, 2018

@@ -30,6 +30,12 @@ var (
"Whether to ignore TLS verification checks (cert validity, hostname). *DO NOT USE IN PRODUCTION*.",
)

flagMaxMsgSize = pflag.Int(
"backend_max_msg_size",
4194304,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this equivalent to the current max? https://github.com/grpc/grpc-go/blob/v1.8.2/server.go#L54. Also please add a reference to this line on github (i.e., copy this link). Note that the link is for v1.8.2 which is the version we've locked in Gopkg.toml.

Copy link
Contributor Author

@nevi-me nevi-me Sep 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean to be explicit as 1024 * 1024 * 4 instead of the product? I'll add the link

go/grpcwebproxy/backend.go Outdated Show resolved Hide resolved
flagMaxMsgSize = pflag.Int(
"backend_max_msg_size",
4194304,
"Maximum message size limit. If not specified, the default of 4MB will be used.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

receive size limit.

go/grpcwebproxy/backend.go Outdated Show resolved Hide resolved
go/grpcwebproxy/backend.go Outdated Show resolved Hide resolved
Copy link
Contributor

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This failed to compile. I think you may need to use gRPC.WithDefaultCallOptions or something like that

@nevi-me
Copy link
Contributor Author

nevi-me commented Sep 15, 2018

Apologies, I still struggled to compile the binary after dep ensure, so I didn't see that I was passing the wrong type.

I've also added some documentation for the option on the README, so we can take care of that while we're at it.

Copy link
Contributor

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice, the README addition is great!

@johanbrandhorst
Copy link
Contributor

Let's just see how the build goes but the code LGTM.

@nevi-me
Copy link
Contributor Author

nevi-me commented Sep 15, 2018

I might not be able to immediately use the updated binary, my go won't compile, errors with $GOPATH. Do you know if Travis generates build artifacts?

@nevi-me nevi-me changed the title WIP: Add option to increase max message size Add option to increase max message size Sep 15, 2018
@johanbrandhorst
Copy link
Contributor

I don't think Travis does. It really should not be hard to build if you have go installed already. Have you checked the README in the grpcwebproxy folder? It has a step by step guide.

@johanbrandhorst johanbrandhorst merged commit b89ec63 into improbable-eng:master Sep 15, 2018
@johanbrandhorst
Copy link
Contributor

Thanks for your contribution!

@nevi-me
Copy link
Contributor Author

nevi-me commented Sep 15, 2018

I got dep installed, but when I go install ./go/grpcwebproxy, go complains about my $GOPATH. I'll have a look at it tomorrow, I'm retiring for the night (GMT +2). Thanks for holding my hand through the process Johan!

@johanbrandhorst
Copy link
Contributor

😬 this is probably easier to resolve in a chat: there's a gRPC-Web slack you can join and we can debug your setup in there: https://join.slack.com/t/improbable-eng/shared_invite/enQtMzQ1ODcyMzQ5MjM4LWY5ZWZmNGM2ODc5MmViNmQ3ZTA3ZTY3NzQwOTBlMTkzZmIxZTIxODk0OWU3YjZhNWVlNDU3MDlkZGViZjhkMjc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants