Skip to content
This repository has been archived by the owner on Jun 28, 2022. It is now read-only.

py,go,java: remove send/recv msg size limit #2900

Merged
merged 6 commits into from Aug 1, 2019
Merged

py,go,java: remove send/recv msg size limit #2900

merged 6 commits into from Aug 1, 2019

Conversation

noahdietz
Copy link
Contributor

Fixes #2895 for python, Go and Java

Node.js (grpc-js) does not check size limits, therefore it is already unbounded.

@noahdietz noahdietz requested a review from yihanzhen July 31, 2019 19:03
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 31, 2019
@yihanzhen
Copy link
Contributor

Java doesn't seem to support maxOutboundMessageSize :(

@codecov
Copy link

codecov bot commented Jul 31, 2019

Codecov Report

Merging #2900 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2900      +/-   ##
============================================
- Coverage     87.34%   87.34%   -0.01%     
+ Complexity     5854     5853       -1     
============================================
  Files           477      477              
  Lines         23223    23223              
  Branches       2507     2507              
============================================
- Hits          20285    20284       -1     
  Misses         2100     2100              
- Partials        838      839       +1
Impacted Files Coverage Δ Complexity Δ
.../java/com/google/api/codegen/discovery/Schema.java 84.34% <0%> (-0.51%) 42% <0%> (-1%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 679912f...8257e1e. Read the comment docs.

@codecov
Copy link

codecov bot commented Jul 31, 2019 •
edited

Codecov Report

Merging #2900 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2900      +/-   ##
============================================
+ Coverage     87.33%   87.33%   +<.01%     
  Complexity     5858     5858              
============================================
  Files           477      477              
  Lines         23237    23238       +1     
  Branches       2510     2510              
============================================
+ Hits          20295    20296       +1     
  Misses         2100     2100              
  Partials        842      842
Impacted Files Coverage Δ Complexity Δ
...egen/transformer/go/GoGapicSurfaceTransformer.java 100% <100%> (ø) 41 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b20acc...7609d0e. Read the comment docs.

@noahdietz
Copy link
Contributor Author

Oops! I assumed 🤦‍♂️ Fixed!

yihanzhen
yihanzhen approved these changes Jul 31, 2019
View reviewed changes
Copy link
Contributor

@yihanzhen yihanzhen left a comment

Choose a reason for hiding this comment

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

LGTM

@noahdietz noahdietz requested review from pongad and removed request for pongad August 1, 2019 17:15
@noahdietz noahdietz merged commit 5cd7aa4 into googleapis:master Aug 1, 2019
@noahdietz noahdietz deleted the issue-2895 branch August 1, 2019 18:01
@noahdietz
Copy link
Contributor Author

The latter changes to the Go snippets were essentially copied from googleapis/gapic-generator-go#165

This was referenced Aug 1, 2019
Closed
Merged
@busunkim96 busunkim96 mentioned this pull request Aug 2, 2019
Closed
@busunkim96
Copy link
Contributor

I opened grpc/grpc#19810 to see if the restriction could be lifted in gRPC.

tseaver
tseaver reviewed Aug 2, 2019
View reviewed changes
Copy link
Contributor

@tseaver tseaver left a comment

Choose a reason for hiding this comment

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

This PR broke autosynth in Python.

src/main/resources/com/google/api/codegen/py/transport.snip
options={
'grpc.max_send_message_length': -1,
'grpc.max_receive_message_length': -1,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

This line needs to have .items() tacked on (options is expected to be a sequence of two-tuples, not a dict).

@tseaver tseaver mentioned this pull request Aug 2, 2019
Closed
@noahdietz noahdietz mentioned this pull request Aug 2, 2019
Closed
busunkim96 pushed a commit to busunkim96/gapic-generator that referenced this pull request Nov 7, 2019
@noahdietz @busunkim96
py,go,java: remove send/recv msg size limit (googleapis#2900)
15de418
@tswast tswast mentioned this pull request Oct 19, 2020
Closed
@busunkim96 busunkim96 mentioned this pull request Dec 10, 2020
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tseaver tseaver tseaver left review comments

@lukesneeringer lukesneeringer lukesneeringer approved these changes

@yihanzhen yihanzhen yihanzhen approved these changes

Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add config option for maxInboundMsgSize
6 participants
@noahdietz @yihanzhen @busunkim96 @lukesneeringer @tseaver @googlebot