Proposal for some consumer processes improvements #232

hostep · 2019-08-19T20:32:03Z

Problem

The message queue consumer processes can use some improvements.
As requested by @maghamed, here's an architectural proposal based on

Message queue consumer improvements to allow them to waste less resources community-features#180
Create an easy way to kill all currently running message queue consumer processes community-features#181

Most of this document was written from memory from what I've seen a couple of weeks ago. Therefore there can potentially be some factual mistakes in here, if that's the case, please let me know!

Solution

See the document, it provides some potential solutions.

Requested Reviewers

@nuzil, @paliarush

magento-cicd2 · 2019-08-19T20:32:07Z

All committers have signed the CLA.

design-documents/asynchronous-operations/consumer-processes-improvements.md

nuzil · 2019-08-20T08:27:40Z

design-documents/asynchronous-operations/consumer-processes-improvements.md

+
+These options should be configurable per consumer type.
+Some sensible defaults could be set in the [`queue_consumer.xml`](https://devdocs.magento.com/guides/v2.3/extension-dev-guide/message-queues/config-mq.html#queueconsumerxml) file for some of these options.  
+Next to that, developers or shopowners should be able to override these values per consumer type. At least being able to override it in the `app/etc/env.php` file would be nice, but a backend interface for making these things configurable would also be very nice (but can maybe be done in a later phase?).


Agree with this point. Will be nice to have UI for available consumers, their shedule and even about amount of processed messages

tariqjawed83 · 2019-08-27T22:57:55Z

design-documents/asynchronous-operations/consumer-processes-improvements.md

+These options should be configurable per consumer type.
+Some sensible defaults could be set in the [`queue_consumer.xml`](https://devdocs.magento.com/guides/v2.3/extension-dev-guide/message-queues/config-mq.html#queueconsumerxml) file for some of these options.  
+Next to that, developers or shopowners should be able to override these values per consumer type. At least being able to override them in the `app/etc/env.php` file would be nice, but a backend interface for making these things configurable would also be very nice (but can maybe be done in a later phase?).
+


only-spawn-when-message-available

max-idle-time

These are good options, and the combination should work

tariqjawed83 · 2019-08-27T23:09:14Z

design-documents/asynchronous-operations/consumer-processes-improvements.md

+
+The suggestion here would be to give deployment scripts the option to signal the running consumer proccesses to handle their current messages and then kill themselves as soon as possible.  
+We already have the poison pill functionality which could be used here, but the problem with that one is that it will only be checked when a new message appears in the queue. If no messages appear in the queue during a deploy, and we only update the poison pill version, the consumer will still keep running because it doesn't check for the poison pill version until a new message appears.  
+So I would suggest some kind of dummy message to be created in each queue which only purpose is for the consumers to check the poison pill version and that message then should get removed again from the queue and nothing should be done with it.


If 'max-idle-time' is implemented as suggested above, then this new message to each queue might not be required; since after specified max idle time, the consumer process will stop by itself; and then rerun when the new message arrives by 'consumer_runner'. So it should solve the underlying issue.

That's true on the one hand, but on the other, there maybe cases where you explicitly want to stop consumer processes at a specific time, for example during a deploy to put a new version of the code online.
I know some people stop the crontab during deploys, so when you kill consumers using this new command, and crons are disabled, no new ones will spawn until the crons are enabled again after the deploy was finished.
It "feels" more clean to do a deploy in this way.

But personally I'm fine with leaving this out if it's not wanted.

@hostep that's right, with the existing poison pill functionality the consumer would stop for the indicated code changes. And then we can follow the existing process as you mentioned disabling the crons until the deployment is finished.

@nuzil just to confirm, this proposed solution 'only-spawn-when-message-available' for 'consumers_runner' is something that can be done?

Leaving this in here for now, it doesn't sound like there is a consensus reached yet.
Let me know if I misunderstood this.

let me confirm and get to some consensus with @nuzil ; the overall approach seems to work fine, just want to make sure, we are all on same page.

Sorry, I didn't realize we still need to solve problem 2 for people not wanting to use the max-idle-time flag. So I've added it again and removed the dummy message part.

Is this ok now?

@hostep I would agree with you on this, 'max-idle-time' would be more flexible approach to dispose the currently running queue process; If its sets to '0' then it could potentially act like no waiting; so it gives flexibility to the site administrator.

Indeed.

Also, be aware that the proposal is for these configuration options to exist per consumer type. The new consumers_wait_for_messages configuration option on the other hand is now implemented to act globally for all consumers, which is also not flexible enough in my opinion.

So it looks like most of it is now clear and we are on the same page, what are the next steps to move this proposal forward?

Thanks!

@hostep I can help this move forward, although wanted to check how we are planning to implement this feature as part of ConsumersRunner job, how much refactoring this may require? wanted to do quick feasibility check, since ConsumersRunner is not dealing with messages directly as of now. Do you have some strategy in mind?

Hi @tariqjawed83

Thanks for bringing this up during the weekly architectural meeting, you explained it very well! 🙂

As for the implementation details, I haven't looked into that yet since I'm not super familiar with the code (yet). If you do want some help looking into this, let me know (I'm also available on Slack most of the time if that makes it easier to communicate).

…ion about MC-19250. Some other small improvements.

design-documents/asynchronous-operations/consumer-processes-improvements.md

…ll version, removed the dummy message suggestion.

keithbentrup · 2019-09-05T14:13:12Z

Note: This is a tangent about naming, not about the proposal which I'm still digesting ...

I'm just glancing through this, and I'm surprised that we have a table named queue_poison_pill (who came up with that and what were they thinking?), and then someone was proposing reusing the poison pill name in the CLI? Thankfully, using "suicide" in the CLI was already quashed.

C'mon, we're writing code here that we want to be simple, clear, and as unambiguous as possible, not a dramatic novel. How about just queue_restart_version? A poison pill would presumably kill something, and there's no connotation that it would be "reborn" (to continue anthropomorphizing - which I don't think we should).

Keep it clear and simple. Words in our code and interfaces matter, and Magento can definitely improve both.

hostep · 2019-09-05T18:09:32Z

Thanks for the feeback @keithbentrup!

Totally makes sense to keep wording simple. This was just a proposal, nothing is set in stone as of yet (I hope?), it was certainly not my intention to force the naming I came up with to actually being used, that's up to the persons actually writing out the specs or implementing it (I think?)

… and new CLI command.

tariqjawed83 · 2019-09-09T19:56:06Z

thanks @hostep for all your effort,

Luwdo · 2019-10-17T14:18:16Z

design-documents/asynchronous-operations/consumer-processes-improvements.md

+
+The problem with this option, is that every time the cronjob `consumers_runner` runs, it will spawn a new consumer process, the consumer checks if messages are available and if none found it will kill itself. So this means it will spawn unneeded processes which take up memory, live for a very short period and then disappear again.  
+
+In this proposal, the flag `only-spawn-when-message-available` would run logic to check *before* the consumer process gets spawned and not *after*.


I looked into implementing only-spawn-when-message-available and it seems it would require one of the following:

Add and implement for both queue types a function that can get a count of the queued messages.

Just dequeue a message in StartConsumerCommand core the consumer if we get no message then don't spawn a process. If we do get a message, requeue it using the queue reject function then spawn a process to handle it.

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.

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.

I thought that getMessages and basic_get cause the message to get locked because you are claiming the message for your to process?

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 🙂

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.

hostep · 2019-10-17T19:39:53Z

Just for the record, a proof of concept for the only-spawn-when-message-available flag is available over here: hostep/magento2@d93d288...poc-consumer-improvements

nuzil reviewed Aug 20, 2019

View reviewed changes

design-documents/asynchronous-operations/consumer-processes-improvements.md Outdated Show resolved Hide resolved

nuzil reviewed Aug 20, 2019

View reviewed changes

design-documents/asynchronous-operations/consumer-processes-improvements.md Outdated Show resolved Hide resolved

nuzil reviewed Aug 20, 2019

View reviewed changes

design-documents/asynchronous-operations/consumer-processes-improvements.md Show resolved Hide resolved

nuzil reviewed Aug 20, 2019

View reviewed changes

design-documents/asynchronous-operations/consumer-processes-improvements.md Outdated Show resolved Hide resolved

nuzil reviewed Aug 20, 2019

View reviewed changes

paliarush assigned tariqjawed83 Aug 22, 2019

Proposal for some consumer processes improvements

653ecd4

hostep force-pushed the consumer-processes-improvements branch from 7146faa to 653ecd4 Compare August 25, 2019 14:28

tariqjawed83 reviewed Aug 27, 2019

View reviewed changes

Removed unnecessary options 'max-time' and 'memory-limit'. Added sect…

927dde0

…ion about MC-19250. Some other small improvements.

roma-glushko reviewed Sep 1, 2019

View reviewed changes

design-documents/asynchronous-operations/consumer-processes-improvements.md Outdated Show resolved Hide resolved

Reworked problem 2, only leave the suggestion to update the poison pi…

6aeb6b3

…ll version, removed the dummy message suggestion.

hostep force-pushed the consumer-processes-improvements branch from 5f193b7 to 6aeb6b3 Compare September 1, 2019 15:46

tariqjawed83 mentioned this pull request Sep 3, 2019

September 4, 2019 #258

Closed

Added small remark around the naming of the new configuration options…

10f0c86

… and new CLI command.

tariqjawed83 merged commit a12415f into magento:master Sep 9, 2019

Luwdo reviewed Oct 17, 2019

View reviewed changes


		The problem with this option, is that every time the cronjob `consumers_runner` runs, it will spawn a new consumer process, the consumer checks if messages are available and if none found it will kill itself. So this means it will spawn unneeded processes which take up memory, live for a very short period and then disappear again.

		In this proposal, the flag `only-spawn-when-message-available` would run logic to check before the consumer process gets spawned and not after.

Proposal for some consumer processes improvements #232

Proposal for some consumer processes improvements #232

Uh oh!

Conversation

hostep commented Aug 19, 2019

Problem

Solution

Requested Reviewers

Uh oh!

magento-cicd2 commented Aug 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tariqjawed83 Aug 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tariqjawed83 Aug 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

keithbentrup commented Sep 5, 2019

Uh oh!

hostep commented Sep 5, 2019

Uh oh!

tariqjawed83 commented Sep 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hostep Oct 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Luwdo Oct 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hostep commented Oct 17, 2019

Uh oh!

Uh oh!

magento-cicd2 commented Aug 19, 2019 •

edited

Loading

tariqjawed83 Aug 27, 2019 •

edited

Loading

tariqjawed83 Aug 27, 2019 •

edited

Loading

hostep Oct 17, 2019 •

edited

Loading

Luwdo Oct 18, 2019 •

edited

Loading