[12.x] Fix SQS FIFO and fair queue support #57080
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR #56763 added initial support for SQS FIFO and fair queues. However, there are a couple issues with the implementation.
Issues
The extra options are only added if the queue name ends with ".fifo". This means that fair queues are not actually supported, since the
MessageGroupId
option will only be sent on FIFO queues.The extra options are only added if a job object is dispatched. If the string notation is used to dispatch to a FIFO queue, the extra options will not be added and the code will throw an exception (ex:
Queue::push('ProcessPodcast@handle')
).Depending on how the job is dispatched, the queue name passed into the
getQueueableOptions()
method may benull
. When this is the case, the method will not recognize the queue as a FIFO queue, the extra options will not be added, and the job dispatch will throw an exception.ProcessPodcast::dispatch($podcast)
will attempt to dispatch to the default FIFO queue. However, since the queue name wasn't specified for the job,$queue->push()
will be called with anull
queue name, which will be passed into thegetQueuableOptions()
method.If a FIFO job is dispatched with a delay, the code will throw an exception. FIFO queues do not support per-job delays. Delays are defined at the queue level in AWS. Therefore, FIFO queues do not support the
DelaySeconds
option and will throw an exception if present.Assumptions
When resolving item 2, we need to determine a message group id to use since there is no dispatched object to provide one. There are two options:
I chose to put every string job onto the same message group id, using the name of the queue as the message group id to use. I made this assumption so that FIFO processing would maintained for all string jobs if the user is dispatching string jobs. If we used a unique message group id instead, this would not ensure FIFO processing for the dispatched jobs, which is probably not what was intended considering the use of the SQS FIFO queue.
Resolutions
I would have preferred to create separate PRs to address these issues, but they're pretty much all related to the same method, so I just created the one.
I have added tests that highlight the issues and show they've been fixed.
Extra Considerations
I allude to this in one of the comments in the code, but the current implementation will always generate a unique deduplication id if the job does not define the
deduplicationId()
method. If the user enables content-based deduplication on the queue in AWS, they will not be able to take advantage of this unless they define adeduplicationId()
method that returns an empty value. This is not very intuitive, and we may want a more direct solution for this. However, that is not within the scope of this PR.Please let me know if you need me to make any changes or have any questions. Also, for full disclosure, I am the maintainer of the https://github.com/shiftonelabs/laravel-sqs-fifo-queue package.