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

fix(pubsub): include request overhead when computing publish batch size overflow #9911

Merged
merged 5 commits into from
Dec 5, 2019

Conversation

plamut
Copy link
Contributor

@plamut plamut commented Dec 3, 2019

Fixes #7108.

This PR fixes the logic that computes the publish batch size overflow, taking the total request message size overhead into account. The improved logic prevents the server-side errors simular to the following:

google.api_core.exceptions.InvalidArgument: 400 The value for request_size is too large. You passed 10000096 in the request, but the maximum value is 10000000.

How to test

(see also the system test in this PR)

  • Create a pubisher client with BatchSettings.max_bytes substantially larger than 10_000_000, and BatchSettings.max_latency to one second (so that the publish autocommit does not kick in too soon).
  • Quickly publish a few sizable messages to a topic. Their total size should slightly exceed 10_000_000 bytes.

Actual result (before the fix):
The backend responds with a "400 InvalidArgument" error.

Expected result (after the fix):
All messages are successfully published (the code splits them into multiple publish batches).

PR checklist

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

The maximum allowed size for a PublishRequest on the backend is lower
than a mere sum of the byte sizes of individual messages.

This commit adjusts the batch size overflow calculation to account for
this overhead. It also caps the effective maximum BatchSetting.max_size
value to 10_000_000 bytes (the limit on the backend).

(credit also to GitHub @relud for outlining the main idea first in the
issue description)
@plamut plamut added the api: pubsub Issues related to the Pub/Sub API. label Dec 3, 2019
@plamut plamut requested a review from pradn December 3, 2019 22:35
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 3, 2019
Copy link
Contributor

@software-dov software-dov left a comment

Choose a reason for hiding this comment

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

Judgement call on your part to make a distinct exception class for test_publish_single_message_size_exceeeds_server_size_limit or leave as is, otherwise LGTM.

@plamut
Copy link
Contributor Author

plamut commented Dec 4, 2019

@software-dov Thanks for the quick review, addressed the last outstanding comment.

Copy link
Contributor

@software-dov software-dov left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsub Issues related to the Pub/Sub API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PubSub: check batch max bytes against request byte size
3 participants