PubSub: check batch max bytes against request byte size

[relud]

Is your feature request related to a problem? Please describe.

google.cloud.pubsub_v1.publisher._batch.thread.Batch enforces max bytes on the sum of PubsubMessage.ByteSize for each message in the batch, but the PublishRequest created by Batch.client.publish is larger than that. As a result I need to specify a non default max bytes to guarantee batches create valid requests.

Describe the solution you'd like

Enforce max bytes on the size of the PublishRequest created by Batch.client.publish:

diff --git a/pubsub/google/cloud/pubsub_v1/publisher/_batch/thread.py b/pubsub/google/cloud/pubsub_v1/publisher/_batch/thread.py
index f187024b7c..67fc04841a 100644
--- a/pubsub/google/cloud/pubsub_v1/publisher/_batch/thread.py
+++ b/pubsub/google/cloud/pubsub_v1/publisher/_batch/thread.py
@@ -76,7 +76,7 @@ class Batch(base.Batch):
 # any writes to them use the "state lock" to remain atomic.
 self._futures = []
 self._messages = []
- self._size = 0
+ self._size = types.PublishRequest(topic=topic, messages=[]).ByteSize()
 self._status = base.BatchStatus.ACCEPTING_MESSAGES

 # If max latency is specified, start a thread to monitor the batch and
@@ -281,7 +281,7 @@ class Batch(base.Batch):
 if not self.will_accept(message):
 return future

- new_size = self._size + message.ByteSize()
+ new_size = self._size + types.PublishRequest(messages=[message]).ByteSize()
 new_count = len(self._messages) + 1
 overflow = (
 new_size > self.settings.max_bytes

Describe alternatives you've considered

I can set max bytes to guarantee sufficient overhead, but I feel like it would be better if I didn't need to and this may result in fewer batches.

Additional context

see also #7107, which suggests adding an option to enforce this setting even when the batch is empty.

[kir-titievsky]

At the very least, we should ensure that the default settings do not result in requests sizes that are too large.

[sduskis]

@kir-titievsky, it looks like the Python client currently behaves like the Java client. This feature request would constitute a departure in the logic between clients. It's certainly something to consider, but I'm going to remove the "triaged for GA" label for now.

[kir-titievsky]

hm... I would venture to say this is a bug in both Java and Python, rather than FR, in that the client's batching API does not take into consideration a known property of the service. But I agree that this is not GA blocking.

[tseaver]

@kir-titievsky Based on my somewhat rusty memory, we actually did raise early on when a single message was larger than max_bytes (#4872), and re-worked it (#5343) per request (#4608): see @theacodes comment that we needed to match Java / Go.

[sduskis]

@tseaver, this issue is about how we calculate the size of a PublishRequest, and not what we do when the size is too large.

The Java equivalent of types.PublishRequest(topic=topic, messages=[]).ByteSize() is 0. The smallest value of types.PublishRequest(messages=[message]).ByteSize() (assuming message is empty) is 2, and it goes up by 2 + messageSize for every message.

If that holds up in all languages, then the current calculations are off by len(messages) * 2 bytes. @kir-titievsky, I personally think that we'd have to consider this a "new feature" for all languages, and that this ought not be a GA blocking feature.

[kir-titievsky]

Agreed on not blocking GA on this.

…

On Tue, May 21, 2019, 8:36 AM Solomon Duskis ***@***.***> wrote: @tseaver <https://github.com/tseaver>, this issue is about how we calculate the size of a PublishRequest, and not what we do when the size is too large. The Java equivalent of types.PublishRequest(topic=topic, messages=[]).ByteSize() is 0. The smallest value of types.PublishRequest(messages=[message]).ByteSize() (assuming message is empty) is 2, and it goes up by 2 + messageSize for every message. If that holds up in all languages, then the current calculations are off by len(messages) * 2 bytes. @kir-titievsky <https://github.com/kir-titievsky>, I personally think that we'd have to consider this a "new feature" for all languages, and that this ought not be a GA blocking feature. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7108?email_source=notifications&email_token=AENMYFT5EWNUTY2KBALAACTPWPUD5A5CNFSM4GPBDJB2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODV3YNQI#issuecomment-494372545>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AENMYFUJ6P7J3JKVPRX5B3DPWPUD5ANCNFSM4GPBDJBQ> .

[evanj]

The calculations can be off by more than len(messages)*2 since protobuf length encoding is variable sized. E.g. I have a test program that adds messages of length 999996. The total length of the request goes up by message.ByteSize() + 4 for each message, since it takes 3 bytes to encode the length of the embedded message in the protobuf representation of a repeated field.

[evanj]

FWIW: It appears to me that Java uses an extremely low default batch size threshold of 1000 bytes, if I'm understanding the code correctly: https://github.com/googleapis/google-cloud-java/blob/67668c1411169338374b050eae50ed650e318c54/google-cloud-clients/google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/v1/Publisher.java#L451

This would definitely help avoid the problem, since it will call Publish for basically every message.