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.
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
Proposal for some consumer processes improvements #232
Proposal for some consumer processes improvements #232
Changes from all commits
653ecd4
927dde0
6aeb6b3
10f0c86
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
only-spawn-when-message-available
max-idle-time
These are good options, and the combination should work
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 looked into implementing only-spawn-when-message-available and it seems it would require one of the following:
option 2 would be easier to implement but not ideal behavior.
The mysql queue implementation for Queue::getCount($status) would be easy to implement
It would take the statuses codes from the manager:
MESSAGE_STATUS_NEW = 2;
MESSAGE_STATUS_IN_PROGRESS = 3;
MESSAGE_STATUS_COMPLETE= 4;
MESSAGE_STATUS_RETRY_REQUIRED = 5;
MESSAGE_STATUS_ERROR = 6;
MESSAGE_STATUS_TO_BE_DELETED = 7;
The Amqp would be a little harder to implement
It doesn't have good translations for those statuses, so maybe the function would just have to be
Queue::getPendingCount()
The PhpAmqpLib\Channel\AMQPChannel class does not seem to implement a way to get a count of pending messages in a channel however.
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.
Hi @Luwdo!
My bad, I forgot to mention this over here on Github (this was done in a conversation on Slack), but I created a proof of concept just for this:
hostep/magento2@d93d288...poc-consumer-improvements
The only problem with this is that it introduces a new method to an interface, which is considered backwards incompatible.
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 thought that getMessages and basic_get cause the message to get locked because you are claiming the message for your to process?
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 tested this and that doesn't seem to be the case, the messages still remain available in the queue after those calls. But feel free to test as well 🙂
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.
Must be something else that locks the message. I can give this a go in a development enviorment, I might replace my patch this this option instead. This is a way better option.