-
Notifications
You must be signed in to change notification settings - Fork 1.1k
update queues default message retention value #12458
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
update queues default message retention value #12458
Conversation
🦋 Changeset detectedLatest commit: 5c52f4a The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4039b7e to
6cb017d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
6cb017d to
38c5276
Compare
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
38c5276 to
7cc6994
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
We need a changeset for this fix. Adding one for you. |
f8ff03d to
6bd1a5e
Compare
petebacondarwin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great change to the PR. I assume that there are two defaults internally?
I was worried that it would be a breaking change to change the default.
(We should probably change the PR title, etc).
Yep, free tier and paid users have a different max retention setting value which is handled internally (the default is the same internally as in wrangler, so wrangler doesn't need to send the default). |
.changeset/twenty-bushes-enter.md
Outdated
| "wrangler": minor | ||
| --- | ||
|
|
||
| removing default values for message retention to reflect different limits for free tier queues |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please maybe add the free and paid limits to the changeset as a reminder?
vicb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one nit for the changeset.
Thanks!
| @@ -196,9 +196,6 @@ describe("wrangler", () => { | |||
| if (queueSettings?.delivery_delay === undefined) { | |||
| queueSettings.delivery_delay = 0; | |||
| } | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make the same change for the delivery delay as well? (Maybe in a separate PR though)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the PR, description, and changeset to take out the delivery default as well
Co-authored-by: Victor Berchet <victor@suumit.com>
petebacondarwin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a bunch of suggestions to how we can lay out the changeset a bit differently. But otherwise LGTM.
Co-authored-by: Pete Bacon Darwin <pete@bacondarwin.com>
Co-authored-by: Pete Bacon Darwin <pete@bacondarwin.com>
Co-authored-by: Pete Bacon Darwin <pete@bacondarwin.com>
Fixes #12438
Fixes #12442
Fixes #12460
Fixes MQ-1094
Removing the wrangler default for queues message retention because it is already set to the default value if no value is passed into the API call. Also updating copy around free tier users having a different max retention setting value than paid (message retention must be between 60 and 86400 seconds if on free tier, otherwise must be between 60 and 1209600 seconds).
Otherwise, current workaround is to pass in an explicit message-retention-period-secs value like:
Also removing the wrangler default for delivery delay because it is also already set to the default value if no value is passed into the API call.
Internal discussion
A picture of a cute animal (not mandatory, but encouraged)