Skip to content
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

async_commands: extended configuration proposal #914

Merged
merged 2 commits into from
Aug 20, 2019

Conversation

uro
Copy link
Contributor

@uro uro commented Jul 11, 2019

Hi guys, I use async_commands and I found out that it's missing a few things.

Firstly, we can't prefix run_command queue and because of that, on example: if we use one queue provider with many services it will provide problems as we will push everything to one queue.
Same with many configurations, each of it can have a different prefix.

Then, the second issue: default Symfony process timeout is 60 seconds. after this, the message is requeued and processed again. We don't have any option to extend this time.

In this pull-request I provide a proposal for these two issues:

  • Prefix run_command queue
  • Change the default process timeout

Copy link
Contributor

@Steveb-p Steveb-p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good at first glance. I've skimmed the code for obvious errors and it looks good to me. Will come back to it in the evening if time allows.

@uro
Copy link
Contributor Author

uro commented Jul 15, 2019

Hey @Steveb-p. Did you have any chance to look at it? The build is failing from 3rd party reason. I'd like to know if we can do some progress with it.

Cheers,
Arkadiusz

@Steveb-p
Copy link
Contributor

@uro I've re-run the failing test and it is indeed steming from a service failing to start. Green now.

I'd like @makasim to take a quick look on it as well, just to be sure, and I think it's good to go 👍

'command' => Commands::RUN_COMMAND,
'queue' => Commands::RUN_COMMAND,
'client' => $client['name'],
'command' => $client['prefix'].Commands::RUN_COMMAND,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO prefix is misleading, what about to allow configure a whole command and queue names?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default value could be the one from Commands::RUN_COMMAND

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@makasim sounds like a great idea. I'm going to implement the fix

@stale
Copy link

stale bot commented Aug 17, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Aug 17, 2019
@stale stale bot removed the wontfix label Aug 19, 2019
@uro
Copy link
Contributor Author

uro commented Aug 19, 2019

Hey @makasim, please accept my apologies for this late reply. I submitted the change related to the command name.

Could you take a look again?

@Steveb-p Steveb-p requested a review from makasim August 19, 2019 21:24
@Steveb-p
Copy link
Contributor

Steveb-p commented Aug 19, 2019

Looks good to me. There should be no BC breaks because of it so no migration documentation necessary - though docs related to async commands might need an update.

Specifically:
https://github.com/php-enqueue/enqueue-dev/blob/master/docs/bundle/async_commands.md
https://github.com/php-enqueue/enqueue-dev/blob/master/docs/bundle/config_reference.md

@makasim makasim merged commit ee19071 into php-enqueue:master Aug 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants