Skip to content

Conversation

@Xnopyt
Copy link
Collaborator

@Xnopyt Xnopyt commented Nov 10, 2025

This PR adds support for automatically setting MessageGroupId based a property of the job's body.
It will try to use $groupId as either an array key (if the body is an array) or as an object property name (if it's an object) and fallback to not setting a group Id when not set or the body is not an object/array.
To maintain backwards compatibility, if $groupId is not set on the jobqueue the check never takes place and no MessageGroupId is passed.

This change allows for AWS SQS fair queues to automatically prioritize jobs based on which groups they are coming from which can help mitigate issues with noisy neighbors in multi-tenant uses.

@Xnopyt Xnopyt self-assigned this Nov 10, 2025
@Xnopyt Xnopyt added the minor-version Release SHOULD be minor version increment label Nov 10, 2025
@codecov
Copy link

codecov bot commented Nov 10, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.66%. Comparing base (7db4bc9) to head (5bdfa82).
⚠️ Report is 2 commits behind head on 2.x.x.

Additional details and impacted files
@@             Coverage Diff              @@
##              2.x.x      #17      +/-   ##
============================================
+ Coverage     68.07%   70.66%   +2.58%     
- Complexity      132      138       +6     
============================================
  Files            10       10              
  Lines           473      484      +11     
============================================
+ Hits            322      342      +20     
+ Misses          151      142       -9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tsmgeek
Copy link

tsmgeek commented Nov 10, 2025

Would this not be more flexible by adding getMessageGroupID to the Job class/interface rather than read it from the payload?

@Xnopyt
Copy link
Collaborator Author

Xnopyt commented Nov 10, 2025

Would this not be more flexible by adding getMessageGroupID to the Job class/interface rather than read it from the payload?

Job and JobInterface are also used by beanstalk and the dbscheduler whilst MessageGroupId only does something when an SQS JobQueue is in use. This would also break in the dbscheduler as storing a job would lose track of the group id, whereas keeping it in the body allows it to be set when jobs are pulled from the DB without needing to add new DB columns.
Also both the interface changes and dbscheduler changes would IMO warrant a major version (due to breaking changes) and mean we cannot do this as a backport to the 2.x.x branch, which we currently need for PHP 7.4 compatibility.

@tsmgeek
Copy link

tsmgeek commented Nov 10, 2025

Would this not be more flexible by adding getMessageGroupID to the Job class/interface rather than read it from the payload?

Job and JobInterface are also used by beanstalk and the dbscheduler whilst MessageGroupId only does something when an SQS JobQueue is in use. This would also break in the dbscheduler as storing a job would lose track of the group id, whereas keeping it in the body allows it to be set when jobs are pulled from the DB without needing to add new DB columns. Also both the interface changes and dbscheduler changes would IMO warrant a major version (due to breaking changes) and mean we cannot do this as a backport to the 2.x.x branch, which we currently need for PHP 7.4 compatibility.

one way could add JobGroupableInterface and then extend Job in MXA which implements that interface as well

@Xnopyt
Copy link
Collaborator Author

Xnopyt commented Nov 10, 2025

Would this not be more flexible by adding getMessageGroupID to the Job class/interface rather than read it from the payload?

Job and JobInterface are also used by beanstalk and the dbscheduler whilst MessageGroupId only does something when an SQS JobQueue is in use. This would also break in the dbscheduler as storing a job would lose track of the group id, whereas keeping it in the body allows it to be set when jobs are pulled from the DB without needing to add new DB columns. Also both the interface changes and dbscheduler changes would IMO warrant a major version (due to breaking changes) and mean we cannot do this as a backport to the 2.x.x branch, which we currently need for PHP 7.4 compatibility.

one way could add JobGroupableInterface and then extend Job in MXA which implements that interface as well

That still would not solve the issues with DbScheduler as unless it's in the job's body it won't get stored. Unless we are happy to not support fair queues for jobs which get stored to DB, which is currently any job with a delay over 5 minutes in our setup.

@TonyCXP TonyCXP self-requested a review November 10, 2025 17:20
@Xnopyt Xnopyt merged commit d608f07 into phlib:2.x.x Nov 11, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

minor-version Release SHOULD be minor version increment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants