Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new experimental package google_cloud_pubsub for Google Cloud Pub/Sub in Dart, and updates google_cloud_logging and google_cloud_shelf to support request-log correlation using Dart Zones and W3C traceparent headers. Feedback on the new Pub/Sub client identifies potential infinite loops/busy-waiting in _onAckBatch and _onModifyAckBatch when active streams are empty, suggesting a fallback to unary RPCs to prevent high CPU usage and unacknowledged messages.
|
Where are we here? |
bed165c to
774f2c1
Compare
|
/gcbrun |
7 similar comments
|
/gcbrun |
|
/gcbrun |
|
/gcbrun |
|
/gcbrun |
|
/gcbrun |
|
/gcbrun |
|
/gcbrun |
93e4ba5 to
5f98a79
Compare
|
/gcbrun |
5f98a79 to
791b1c7
Compare
|
/gcbrun |
791b1c7 to
5ad97bc
Compare
|
/gcbrun |
5ad97bc to
64e494c
Compare
3bbd35e to
a126777
Compare
…astructure - Add BatchingSettings, PublishSettings, and AckSettings. - Add background message batching for Topic.publish. - Add background acknowledgment and deadline modification batching for Subscription.acknowledge and Subscription.modifyAckDeadline. - Add close() lifecycle methods on Topic and Subscription. - Add publishMessages on PubSub for publishing multiple messages in a single RPC. - Add RetrySettings and runWithRetry exponential backoff retry infrastructure.
6415f92 to
3545789
Compare
brianquinlan
left a comment
There was a problem hiding this comment.
You didn't want to add any end-to-end tests?
| import 'package:meta/meta.dart'; | ||
|
|
||
| /// Settings for configuring retry logic with exponential backoff. | ||
| final class RetrySettings { |
There was a problem hiding this comment.
In other language's ecosystem's, this goes elsewhere so that I can be reused across packages:
https://docs.cloud.google.com/pubsub/docs/retry-requests#retry_a_message_request
I guess that it doesn't cost us anything to have this here for now and then move it later (we can use export for backwards compatibility).
There was a problem hiding this comment.
Good call — since google_cloud_pubsub hasn't been published yet, I went ahead and moved this now rather than deferring. RetryRunner and ExponentialRetry now live in package:google_cloud_rpc/retry.dart, and both google_cloud_storage and google_cloud_pubsub re-export and use them (closes #346).
| import 'package:meta/meta.dart'; | ||
|
|
||
| /// Settings for batching operations. | ||
| final class BatchingSettings { |
There was a problem hiding this comment.
Interesting, this is a generic class in Java:
https://docs.cloud.google.com/java/docs/reference/gax/latest/com.google.api.gax.batching.BatchingSettings.html
But PubSub specific in Python:
https://docs.cloud.google.com/python/docs/reference/pubsub/latest/google.cloud.pubsub_v1.types.BatchSettings
Maybe add some of the documentation details from the Python version. max_bytes in particular seems helpful:
The maximum total size of the messages to collect before automatically publishing the batch, including any byte size overhead of the publish request itself. The maximum value is bound by the server-side limit of 10_000_000 bytes.
Or maybe the part about overhead is not true?
There was a problem hiding this comment.
You were right to be suspicious, and it turned out to be worth chasing — thank you for pushing on it. The overhead sentence was not true for our implementation, and making it true uncovered a second, worse problem. Both are now fixed.
The publish path
We were counting only data.length plus the UTF-8 length of each attribute key and value. The server, though, validates PublishRequest.ByteSizeLong() — the fully serialized request, including the topic field, the per-message tag and length prefix, and the attribute-map framing. So maxBytes was measuring something the server doesn't.
At the 1 MiB default this is harmless (0.06% overhead, and we sit 10× under the limit), but it is not harmless if you raise it. Measured:
maxBytes |
message shape | counted | real wire | vs 10,000,000 |
|---|---|---|---|---|
| 1 MiB (default) | 10 KiB | 1,024,000 | 1,024,663 | fine |
| 10,000,000 | 1 KiB | 9,999,360 | 10,058,013 | +58 KB over |
| 10,000,000 | 100 B | 10,000,000 | 10,400,063 | +400 KB over |
| 10,000,000 | 10 B + 5 attributes | 9,999,910 | 12,058,778 | +2 MB over (20.6%) |
An oversized request fails with a non-retryable INVALID_ARGUMENT and takes the whole batch with it.
Looking at the other clients, we were the outlier: Python and Java count the framing exactly, Go approximates it and clamps, Node counts naively like we did but clamps to 9 MiB for ~563 KB of headroom. We had neither. (Your instinct to quote Python's docstring was right — that wording is the ecosystem's contract.)
Fixed by counting wire sizes arithmetically without serializing (package:protobuf has no cached-size API). New test/wire_size_test.dart pins the arithmetic to real writeToBuffer() output across varint boundaries, empty payloads, empty attribute keys/values, multi-byte UTF-8, and 2,000 random messages.
The acknowledgment path — the one I didn't expect
AckSettings shares BatchingSettings with publishing, so the ack and modack batchers inherited maxBytes = 1 MiB. The server limit for Acknowledge and ModifyAckDeadline is 512 KB. The default was 2× the limit.
Nothing broke in practice only because maxMessages: 100 always binds first. Raise maxMessages and you'd hit it — and because _onAckBatch swallows errors for fire-and-forget callers, the symptom would not be an exception but every message in the batch being redelivered indefinitely.
AckSettings now defaults maxBytes to 512 KB, and ack batches are measured with the same exact accounting.
Also
-
Settings above the applicable server limit now throw an
ArgumentError
rather than silently producing a doomed request. Python, Go and Node all
clamp instead; clamping felt worse here, because
BatchingSettings(maxBytes: 50000000)silently becoming 10,000,000 gives you
no signal that you asked for something impossible.There is one wrinkle this created.
maxBytesdefaults to 1 MiB, which is
right for publishing but above the 512,000-byte acknowledgment limit — so
AckSettings(batching: BatchingSettings(maxMessages: 50))would throw over a
byte limit the caller never expressed an opinion about.BatchingSettings
therefore records privately whethermaxByteswas passed explicitly, and only
throws if it was; an unspecified default is quietly narrowed. The public field
stays a non-nullableintwith an unchanged default, so the API shape doesn't
change. The alternative — makingmaxBytesanint?— is cleaner
conceptually but makes "what is the default" unanswerable from the field, and
validating in the constructor is impossible since the applicable limit depends
on which RPC the settings end up feeding. -
maxMessagesgets the same treatment against the 1,000-message publish limit.
It needs no explicitness flag: its default of 100 is below every count limit,
so anything over is necessarily deliberate. -
The
+4in the modack batcher is now the exact varint size of the deadline. While fixing it I noticed the comment I'd just added was wrong: onlyStreamingPullRequesthas the parallel deadline list; a unaryModifyAckDeadlineRequesthas a single shared deadline. The comment says so now, and notes that this makes the estimate conservative on the unary path.
Regression tests on both paths: for publishing, 4,000 small messages with five attributes each (the worst case for framing), asserting that no request exceeds maxBytes; for acknowledgments, test/subscription_batching_test.dart measures every AcknowledgeRequest and ModifyAckDeadlineRequest emitted with writeToBuffer(), including batches that fan out across multiple distinct deadlines.
One thing I did not do: reject a single message too large to ever fit. Python raises MessageTooLargeError client-side for this; we still let the server reject it. Happy to add that if you'd like it.
|
|
||
| /// Returns whether [error] is considered a retryable error. | ||
| @internal | ||
| bool isRetryable(Object error) { |
There was a problem hiding this comment.
Probably we can't share code with https://github.com/googleapis/google-cloud-dart/blob/main/pkgs/google_cloud_storage/lib/src/retry.dart , right?
There was a problem hiding this comment.
We can! I unified both packages on storage's RetryRunner / ExponentialRetry design in package:google_cloud_rpc/retry.dart (google_cloud_storage re-exports it for backwards compatibility):
- Added
jitter(defaulting to0.0for storage,0.2indefaultPubSubRetry) and an injectableisRetryablepredicate toExponentialRetry. - Since Pub/Sub's client already maps
GrpcErrorstatus codes toServiceExceptions withStatus(code: ...),defaultIsRetryableingoogle_cloud_rpchandles all mapped gRPC and HTTP retryable status codes directly without needing apackage:grpcdependency.google_cloud_pubsubaddsisPubSubRetryable(anddefaultPubSubRetry) to cover rawGrpcErrors in unit tests and Pub/Sub's backoff defaults (100 ms initial delay, 1.3× multiplier, ±20% jitter). - Also fixed a subtle bug in
delaySequence:noRetriesAfteris now computed eagerly whendelaySequence()is called rather than inside thesync*generator body (which previously delayed deadline calculation until the first retry).
| ); | ||
| _modifyAckBatcher = Batcher<_ModifyAckDeadlineRequest>( | ||
| settings: ackSettings.batching, | ||
| itemSize: (request) => request.ackId.length + 4, |
There was a problem hiding this comment.
Maybe add a comment explaining the +4?
There was a problem hiding this comment.
Thanks for questioning this — looking into the +4 made me realize it was
wrong, and fixing it properly meant fixing how the whole batch is measured.
It's gone now, replaced by exact accounting, so rather than explain the
constant I'll explain what replaced it.
Two things were wrong with +4:
- The deadline is a varint, not a fixed 4 bytes.
int32on the wire is
base-128 varint encoded, so a typical 600-second deadline costs 2 bytes, not
4. It's nowvarintSize(request.ackDeadlineSeconds). - My justification for it was wrong too. I'd written that
ModifyAckDeadlinecarries amodifyDeadlineSecondslist parallel to the
ack ID list. That's only true ofStreamingPullRequest— a unary
ModifyAckDeadlineRequesthas a single sharedackDeadlineSeconds
(field 3). One batcher feeds both paths, so it still charges each ack ID for
its own deadline, but the comment now says that this makes the estimate
conservative on the unary path rather than exact.
More importantly, the old accounting counted only ackId.length and nothing
else — not the per-ack-ID field tag and length prefix, and not the subscription
name that every request carries. That mattered much more than the +4: see my
reply on batching.dart for the full story, but the short version is that
AckSettings was sharing publishing's 1 MiB maxBytes against a server limit
of 512,000 bytes for Acknowledge and ModifyAckDeadline.
The batcher now seeds its running total with the serialized size of the
subscription field and charges each ack ID its real framing, and
test/subscription_batching_test.dart measures every request the client
actually emits with writeToBuffer(). Reverting to the old accounting fails
three of those tests.
…tests Addresses review feedback on #292: - Document what `BatchingSettings.maxBytes` actually measures. Only the items themselves are counted, so unlike the Python client the per-request protobuf and gRPC overhead is excluded. Also document that an oversized item is sent alone in a batch that exceeds the limit, and note the server-side 10,000,000 byte limit that is not enforced here. - Correct the default's unit comment: 1024 * 1024 is 1 MiB, not 1 MB. - Explain the `+ 4` in the modify-ack-deadline item size: each ack ID is accompanied by its own int32 deadline in a parallel list. - Add `batching_e2e_test.dart`, covering behaviour that the fake-based unit tests cannot: that a batched publish arrives as N distinct messages with N distinct server-assigned ids, that publishes spanning several batches all arrive, that `Topic.close()` flushes buffered messages, and that a batched acknowledgment actually reaches the server. Each test was verified to fail when the behaviour it covers is broken. - Add a TODO referencing #346 for sharing retry logic with `google_cloud_storage`, and clarify that `totalTimeout` is the wall-clock budget across all attempts rather than a per-attempt cap. TAG=agy CONV=4ecd3490-bbbe-499e-90e5-07ea5e14a460
Batching counted only the items themselves — message payloads plus attribute key and value bytes, or bare ack ID lengths. Pub/Sub validates the *serialized* request (`PublishRequest.ByteSizeLong()`), which also includes the topic or subscription field, the per-item tag and length prefix, and the attribute map framing. `maxBytes` was therefore measuring something the server does not. At the 1 MiB default the gap is 0.06% and harmless. It is not harmless once `maxBytes` is raised: with it set to the documented 10,000,000 the real request ran between 58 KB and 2 MB over the limit depending on message shape, and an oversized request fails with a non-retryable INVALID_ARGUMENT that takes the whole batch with it. Among the official clients we were the only one with neither exact accounting nor a safety clamp: Python and Java count the framing exactly, Go approximates and clamps, Node counts naively but clamps to 9 MiB. More seriously, `AckSettings` shares `BatchingSettings` with publishing, so the acknowledgment batchers inherited a 1 MiB ceiling against a 512 KB server limit for `Acknowledge` and `ModifyAckDeadline` — twice what is allowed. Only `maxMessages: 100` binding first kept this from firing, and because `_onAckBatch` suppresses errors for fire-and-forget callers the symptom would not have been an exception but indefinite redelivery of every message in the batch. Sizes are computed arithmetically rather than by serializing: `itemSize` runs on every add, and `package:protobuf` offers no cached size, only `writeToBuffer`. Measured at 0.30 µs versus 0.29 µs for the old count, against 0.98 µs to serialize. Also caps settings to the applicable server limit rather than letting them produce a request that cannot succeed, and corrects the modify-ack-deadline comment: only `StreamingPullRequest` carries a deadline list parallel to its ack IDs, so the estimate is conservative on the unary path. TAG=agy CONV=4ecd3490-bbbe-499e-90e5-07ea5e14a460
The CHANGELOG entry claimed resilient streaming pull and stream-routed acknowledgment batches, neither of which exists on this branch — those bullets belong to the stacked PR and were dragged in when the commit was moved between branches. Removed, and the batching entry now states both caps and the fact that the acknowledgment default dropped from 1 MiB. `BatchingSettings` promised that a batch is "never knowingly sent over the limit" a dozen lines before explaining that a single oversized item is sent alone in a batch that exceeds it. The first is the paragraph a user consults to decide whether they need to size-check their own messages, so it is now explicit that they do. `wire_size_test.dart` asserted against a local copy of the size function labelled "Mirrors `Topic._publishedMessageSize`", so a change to the real one would have left all 2,000 random cases green. Moved that function into `wire_size.dart` as `publishRequestMessageSize` and pointed both the batcher and the test at it. The acknowledgment path had no byte-accounting tests at all, though it is the path where a mistake is invisible: acknowledgments are fire-and-forget, so an oversized request surfaces as redelivery rather than an exception. Added `subscription_batching_test.dart`, which measures every recorded request with `writeToBuffer()`. Its limits are deliberately tight — the fixed per-request cost has to be a large enough share of `maxBytes` for its omission to change the batch boundary — and reverting to the old accounting fails three of them. Also closes a one-byte under-count: a unary `ModifyAckDeadlineRequest` carries one shared `ackDeadlineSeconds`, whose tag the per-ack-ID estimate only covered for groups of two or more. Its tag is now charged in the base size. `varintSize` guarded its precondition with an assert alone, so a release build would have returned 1 for a negative value that protobuf sign extends to ten bytes. Currently unreachable — every caller validates first — but it now fails in the over-estimating direction, which is the safe one for a size limit. TAG=agy CONV=4ecd3490-bbbe-499e-90e5-07ea5e14a460
Asking for a `maxBytes` or `maxMessages` above the applicable Pub/Sub limit used to be capped silently, matching Python, Go and Node. Throwing is better here: `BatchingSettings` already throws for its other invalid inputs, so clamping was the odd one out, and a capped value left the settings object reporting a size that was not the one in use — `topic.publishSettings.batching .maxBytes` would say 50,000,000 while batches flushed at 10,000,000. The check cannot live in `BatchingSettings`, because which limit applies depends on the request, and one instance may legitimately be shared between `PublishSettings` and `AckSettings`. It is therefore made when the `Topic` or `Subscription` is constructed, which is the first point at which the answer is known, and still well before any RPC. That leaves one case that must not throw. `maxBytes` defaults to 1 MiB, which suits publishing and exceeds the 512,000 bytes an `Acknowledge` request allows, so `AckSettings(batching: BatchingSettings(maxMessages: 50))` would fail over a byte limit the caller never expressed an opinion about. `BatchingSettings` now records whether `maxBytes` was supplied rather than defaulted: a value the caller chose is validated, and the default is narrowed. The field stays a non-nullable `int` with an unchanged default, so this is not visible in the public API. TAG=agy CONV=4ecd3490-bbbe-499e-90e5-07ea5e14a460
…iably Now that #348 has landed, tag `batching_e2e_test.dart` with `google-cloud` so it runs in Cloud Build integration tests as well as the emulator. Also switch resource names to `testResourceName` and use `pullReliably` after `modifyAckDeadlineNow(0)` so eventual consistency on real GCP does not flake on the first pull. TAG=agy CONV=4ecd3490-bbbe-499e-90e5-07ea5e14a460
…y to google_cloud_rpc Unifies retry configuration across google_cloud_storage and google_cloud_pubsub by moving RetryRunner, ExponentialRetry, delaySequence, defaultIsRetryable, and ChecksumValidationException into package:google_cloud_rpc/retry.dart. - google_cloud_storage re-exports package:google_cloud_rpc/retry.dart and ChecksumValidationException for backwards compatibility. - google_cloud_pubsub replaces RetrySettings with RetryRunner / ExponentialRetry and adds defaultPubSubRetry and isPubSubRetryable. TAG=agy CONV=4ecd3490-bbbe-499e-90e5-07ea5e14a460
Ensures custom ExponentialRetry instances passed to Pub/Sub automatically use isPubSubRetryable when no custom predicate was specified. TAG=agy CONV=4ecd3490-bbbe-499e-90e5-07ea5e14a460
TAG=agy CONV=4ecd3490-bbbe-499e-90e5-07ea5e14a460
|
/gcbrun |
You were right to push on this. I added four, and they immediately earned their keep. What's new —
And running the suite found a real bug — in the stacked PR #339, not this one. Tearing down a streaming pull connection called Note on retry e2e coverage: retry-on- |
brianquinlan
left a comment
There was a problem hiding this comment.
This change is 5000 LOC and hand to review. Is these an obvious way to split it up a bit?
@sigurdm do you have a good mental model of what this change is doing? I'm a bit worried that this PR is being driven by AI without us having a good understanding of what it is doing.
| /// | ||
| /// [idempotent operations]: https://docs.cloud.google.com/storage/docs/retry-strategy#idempotency-operations | ||
| const defaultRetry = ExponentialRetry(); | ||
| export 'package:google_cloud_rpc/retry.dart'; |
There was a problem hiding this comment.
Shouldn't you move/delete retry_test.dart?
|
|
||
| import 'test_utils.dart'; | ||
|
|
||
| void main() { |
There was a problem hiding this comment.
Haven't we been combining the e2e tests and the mock tests in one test file for other tests?
Extract RetryRunner, ExponentialRetry (with optional jitter and custom isRetryable predicate), delaySequence, defaultIsRetryable, NoDelayRetry, and ChecksumValidationException from package:google_cloud_storage into package:google_cloud_rpc/retry.dart and re-export from package:google_cloud_storage for backwards compatibility. Move retry_test.dart from google_cloud_storage to google_cloud_rpc. Closes #346 TAG=agy CONV=4ecd3490-bbbe-499e-90e5-07ea5e14a460
…classification Add isPubSubRetryable, defaultPubSubRetry (100 ms initial delay, 1.3x multiplier, ±20% jitter, 1 min maxRetryInterval), and normalizePubSubRetry. Attach Status(code: ...) to mapped ABORTED and DATA_LOSS exceptions in PubSub._mapGrpcError so defaultIsRetryable and isPubSubRetryable classify them accurately. TAG=agy CONV=4ecd3490-bbbe-499e-90e5-07ea5e14a460
Add BatchingSettings, Batcher, and protobuf wire-size helpers, and wire background batching and retrying into Topic.publish via PublishSettings. Combine mock and e2e publish batching tests in test/publish_test.dart. TAG=agy CONV=4ecd3490-bbbe-499e-90e5-07ea5e14a460
Combine mock and e2e tests in acknowledge_test.dart and modify_ack_deadline_test.dart and stack on pubsub-publish-batching. TAG=agy CONV=4ecd3490-bbbe-499e-90e5-07ea5e14a460
TAG=agy CONV=4ecd3490-bbbe-499e-90e5-07ea5e14a460
Implements request batching and exponential backoff retry infrastructure for
package:google_cloud_pubsub.Closes #346
TAG=agy
CONV=4ecd3490-bbbe-499e-90e5-07ea5e14a460