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

Remove client-side enforcement of send/recv message size limit #669

Closed
tswast opened this issue Oct 19, 2020 · 8 comments · Fixed by #704
Closed

Remove client-side enforcement of send/recv message size limit #669

tswast opened this issue Oct 19, 2020 · 8 comments · Fixed by #704
Assignees
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@tswast
Copy link

tswast commented Oct 19, 2020

Some APIs such as the BigQuery Storage API send / receive blocks which are larger than the default block size. This results in a client side error when recieving messages from a large read request googleapis/python-bigquery-storage#78

This is a regression from this fix in the monolith generator: googleapis/gapic-generator#2900, googleapis/gapic-generator#2905

@tswast
Copy link
Author

tswast commented Oct 19, 2020

@tswast
Copy link
Author

tswast commented Oct 19, 2020

I'm not sure that the service config fields are actually respected. I tried:

  1. Update local clone of googleapis
diff --git a/google/cloud/bigquery/storage/v1/bigquerystorage_grpc_service_config.json b/google/cloud/bigquery/storage/v1/bigquerystorage_grpc_service_config.json
index 2efc373b1..75189345e 100644
--- a/google/cloud/bigquery/storage/v1/bigquerystorage_grpc_service_config.json
+++ b/google/cloud/bigquery/storage/v1/bigquerystorage_grpc_service_config.json
@@ -33,7 +33,9 @@
         "retryableStatusCodes": [
           "UNAVAILABLE"
         ]
-      }
+      },
+      "maxResponseMessageBytes": 4294967295,
+      "maxRequestMessageBytes": 4294967295
     },
     {
       "name": [
  1. Run synthtool
export SYNTHTOOL_GOOGLEAPIS=/usr/local/google/home/swast/src/googleapis
python -m synthtool
  1. What changed?
(scratch) swast@tims-cloud.c ~/src/python-bigquery-storage% git status
On branch issue78-max-message-size
Your branch is up to date with 'upstream/master'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
        modified:   synth.metadata

Untracked files:
  (use "git add <file>..." to include in what will be committed)
        bigquery-storage-v1-py.tar.gz

@software-dov software-dov added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Oct 19, 2020
@busunkim96
Copy link
Contributor

Texttospeech seems to run into the same issue. googleapis/python-texttospeech#5.

Do we want the -1/-1 as before or should we use what's provided in the service config @software-dov?

@software-dov
Copy link
Contributor

Good question. I'll do some investigating to see what the other language surfaces do (that is to say, surfaces for the other programming languages, not just other surfaces for Language).
My kneejerk response is that the service config should be providing this information, and that the two bugs Tim implicitly mentions above are intertwined.

@tswast
Copy link
Author

tswast commented Oct 20, 2020

My kneejerk response is that the service config should be providing this information, and that the two bugs Tim implicitly mentions above are intertwined.

There is a piece in the service config spec for these client-side limits, but they don't seem to do anything. Also, the spec seems flawed in that it is per-method, but a channel is created per-client.

@tswast
Copy link
Author

tswast commented Oct 20, 2020

Note: I'm able to patch the channel creation locally with synth.py in googleapis/python-bigquery-storage#79 and plan to release those changes. I can remove those manual fixes once the generator is updated to avoid this problem.

@software-dov
Copy link
Contributor

I can't find a service config where these variables are set. Is there an example somewhere?

@busunkim96
Copy link
Contributor

Hmm, @tswast shared the service config proto: https://github.com/grpc/grpc-proto/blob/5ce8e3e598b805a1e0372062913f24b0715fdefc/grpc/service_config/service_config.proto#L83-L115 so I think in theory it could be added to the JSON.

But it doesn't seem to show up in googleapis: https://github.com/googleapis/googleapis/search?q=max_request_message_bytes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants