googleapis / gapic-generator-python

Generate Python API client libraries from Protocol Buffers.
Apache License 2.0
121 stars 67 forks source link

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

Closed tswast closed 3 years ago

tswast commented 3 years ago

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 https://github.com/googleapis/python-bigquery-storage/issues/78

This is a regression from this fix in the monolith generator: https://github.com/googleapis/gapic-generator/pull/2900, https://github.com/googleapis/gapic-generator/pull/2905

tswast commented 3 years ago

Or looking at the original bug: https://github.com/googleapis/gapic-generator/issues/2895#issuecomment-516938691

Maybe this can be set in the protos for the service via https://github.com/grpc/grpc-proto/blob/5ce8e3e598b805a1e0372062913f24b0715fdefc/grpc/service_config/service_config.proto#L83-L115

tswast commented 3 years ago

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
busunkim96 commented 3 years ago

Texttospeech seems to run into the same issue. https://github.com/googleapis/python-texttospeech/issues/5.

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

software-dov commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

software-dov commented 3 years ago

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

busunkim96 commented 3 years ago

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