MQ-576: Add attempts to QueueMessage #1874
Conversation
a-robinson
requested review from
a-robinson
and removed request for
byule and
harrishancock
There was a problem hiding this comment.
Note this is a breaking change and we are making sure that all clients are updated accordingly before this change gets released.
What about this is a breaking change?
- If the client doesn't send the field but the server expects it, it'll just default to 0
- If the client does send the field but the server doesn't check for it, it'll behave as if the client hadn't sent the field.
Both of which seem fine to me?
| @@ -98,7 +98,8 @@ struct IncomingQueueMessage { | |||
| kj::Date timestamp; | |||
| kj::Array<kj::byte> body; | |||
| kj::Maybe<kj::String> contentType; | |||
| JSG_STRUCT(id, timestamp, body, contentType); | |||
| uint16_t attempts; | |||
There was a problem hiding this comment.
I assume this will be documented in the docs as well, but does this start at 0 for the first attempt or at 1? It'd be worth noting here so that it's in the code as well as in the docs.
Perhaps the ambiguity suggests a different name would be better (previousAttempts, pastAttempts, etc.), but those come with their own downside in terms of wordiness, and they assume that this starts at 0, but judging by your tests I'm guessing this actually starts at 1 on the first attempt.
There was a problem hiding this comment.
Yes, attempts should start at 1. This is consistent with the behavior we already have for pull-based consumers (and the field is also called attempts). We'll definitely document this, but I think the runtime should not care about the semantics of this field, it will simply "forward" whatever value is passed from the queues code (as long as it is an unsigned integer).
There was a problem hiding this comment.
I thought that comments here got automatically picked up and put into the typescript types for public consumption, but now that I look at https://www.npmjs.com/package/@cloudflare/workers-types?activeTab=code I see that most of the types don't have comments on their fields, so I guess it's not a big deal. Plus it'd be nice to merge these before conflicts start happening, so I'll get this in now.
| @@ -146,6 +146,7 @@ struct QueueMessage @0x944adb18c0352295 { | |||
| timestampNs @1 :Int64; | |||
| data @2 :Data; | |||
| contentType @3 :Text; | |||
| attempts @4 :UInt16; | |||
There was a problem hiding this comment.
If the answer to https://github.com/cloudflare/workerd/pull/1874/files#r1534621760 is that the first attempt of a message is meant to be delivered with an attempts of 1, then perhaps we should just default this field to 1 when it isn't explicitly set by the client? By doing:
attempts @4 :UInt16 = 1;
There was a problem hiding this comment.
It must be always set by the client (we already made the changes), o/w we may be opening the door to bugs.
This patch updates the queue message capnp schema so that the number of attempts is included within each message, allowing queue consumers to implement backoff logic based on that. Note this is a breaking change and we are making sure that all clients are updated accordingly before this change gets released.
sesteves
force-pushed
the
sesteves/mq-576-add-delivery-attempts
branch
from
4257695
to
5a4beaa
Compare
Regarding (1), I tested it and the attempts field does not default to zero, it throws an error, something like: |
That could be changed by making the But it sounds like that doesn't matter if the queues code is already updated, so let's get this merged before any new conflicts creep in. |
|
Think this change needs Miniflare changes in order to be able to land in local dev, see test failures in cloudflare/workers-sdk#5451 |
This patch updates the queue message capnp schema so that the number of attempts is included within each message, allowing queue consumers to implement backoff logic based on that. Note this is a breaking change and we are making sure that all clients are updated accordingly before this change gets released.