Skip to content

pubsub/gcppubsub: Consider publish request body size limitation when batching#3005

Merged
vangent merged 4 commits into
google:masterfrom
zapo:gcppubsub-enforce-request-body-limitation
Jun 2, 2021
Merged

pubsub/gcppubsub: Consider publish request body size limitation when batching#3005
vangent merged 4 commits into
google:masterfrom
zapo:gcppubsub-enforce-request-body-limitation

Conversation

@zapo

@zapo zapo commented May 20, 2021

Copy link
Copy Markdown
Contributor

This allows configuring a maximum batch size in bytes on the batcher in an attempt to respect the GCP 10MB limitation per publish request_size as documented in https://cloud.google.com/pubsub/quotas.
Happy to add tests if this limitation should be enforced at the batcher level like proposed in this PR.

@google-cla google-cla Bot added the cla: yes Google CLA has been signed! label May 20, 2021
Comment thread pubsub/batcher/batcher.go Outdated
Comment thread pubsub/batcher/batcher.go
Comment thread pubsub/batcher/batcher.go Outdated
Comment thread pubsub/batcher/batcher.go Outdated
Comment thread pubsub/batcher/batcher.go
Comment thread pubsub/driver/driver.go Outdated
Comment thread pubsub/driver/driver.go
Comment thread pubsub/gcppubsub/gcppubsub.go Outdated
@vangent vangent self-assigned this May 26, 2021
@zapo

zapo commented May 31, 2021

Copy link
Copy Markdown
Contributor Author

Thank you for the review @vangent, I believe I addressed all the feedback in my latest push. Please let me know if anything else needed here.

@codecov

codecov Bot commented Jun 2, 2021

Copy link
Copy Markdown

Codecov Report

Merging #3005 (2637c84) into master (ec5930b) will increase coverage by 0.11%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3005      +/-   ##
==========================================
+ Coverage   69.43%   69.54%   +0.11%     
==========================================
  Files         111      112       +1     
  Lines       11684    11700      +16     
==========================================
+ Hits         8113     8137      +24     
+ Misses       2902     2896       -6     
+ Partials      669      667       -2     
Impacted Files Coverage Δ
pubsub/gcppubsub/gcppubsub.go 76.71% <ø> (ø)
pubsub/batcher/batcher.go 97.61% <100.00%> (+0.51%) ⬆️
pubsub/driver/driver.go 100.00% <100.00%> (ø)
blob/s3blob/s3blob.go 89.65% <0.00%> (-0.54%) ⬇️
pubsub/pubsub.go 93.85% <0.00%> (+1.70%) ⬆️
docstore/docstore.go 89.42% <0.00%> (+2.20%) ⬆️

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 ec5930b...2637c84. Read the comment docs.

@vangent vangent merged commit b55c7e8 into google:master Jun 2, 2021
@vangent

vangent commented Jun 2, 2021

Copy link
Copy Markdown
Contributor

Thanks for the contribution!

@zapo zapo deleted the gcppubsub-enforce-request-body-limitation branch June 3, 2021 13:25
tonyhb added a commit to tonyhb/go-cloud that referenced this pull request Mar 18, 2026
We currently estimate the size of the batch and limit batch sizes to <=
10MB for GCP PubSub (google#3005).  This estimate can be wrong (by eg ~30kb).

In these cases, we don't want data loss.  Instead, if the batch is too
large we split the batch in half then send both batches.  This obviously
assumes the estimate isn't off by a factor of 2, which we've never seen
in prod.

Also, ideally, we wouldn't even make the OG request and instead would
inspect the size of the protobuf before sending over the wire.  This
requires changes to Google Cloud's apiv1 driver, so that's out of scope
for gocloud.dev.  In this case, we'll deal with a failing outbound
  request then attempt to recover.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Google CLA has been signed!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants