Skip to content

[Messenger] Fix misleading comment about time-limit #38303

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.

Sign up for GitHub

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

Jump to bottom
Merged
fabpot merged 1 commit into from
Sep 26, 2020
Merged

[Messenger] Fix misleading comment about time-limit #38303

fabpot merged 1 commit into from
Sep 26, 2020

Conversation

@sylfabre
Copy link
Contributor

@sylfabre sylfabre commented Sep 25, 2020

Q A
Branch? master
Bug fix? no
New feature? no
Deprecations? no
Tickets n/a
License MIT
Doc PR n/a

The current explanation of the time-limit option of the messenger:consume command is misleading as it lets you think that this is an enforced hard limit.

For instance, you may think that a command started with --time-limit=10 will stop once 10 seconds are elapsed, no matter what.

Actually, two things happen:

  • Once 10 seconds have elapsed, then the worker won't receive and handle any other message
  • The worker will keep running until the currently handled message is fully handled so it can last way longer than 10 seconds, then it will stop.

I'm not sure this is behavior is actually a bug or not, but the current documentation does not describe the current behavior

@sylfabre sylfabre requested a review from sroze as a code owner September 25, 2020 13:30
nicolas-grekas
nicolas-grekas reviewed Sep 25, 2020
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

I'm not super convinced this is better for now. Any alternative proposal? (I don't :) )

@nicolas-grekas nicolas-grekas added this to the next milestone Sep 25, 2020
@sylfabre sylfabre force-pushed the messenger_doc branch 2 times, most recently from 18e0ff8 to 9478df4 Compare September 25, 2020 13:50
nicolas-grekas
nicolas-grekas reviewed Sep 25, 2020
View reviewed changes
src/Symfony/Component/Messenger/Command/ConsumeMessagesCommand.php
new InputOption('failure-limit', 'f', InputOption::VALUE_REQUIRED, 'The number of failed messages the worker can consume'),
new InputOption('memory-limit', 'm', InputOption::VALUE_REQUIRED, 'The memory limit the worker can consume'),
new InputOption('time-limit', 't', InputOption::VALUE_REQUIRED, 'The time limit in seconds the worker can run'),
new InputOption('time-limit', 't', InputOption::VALUE_REQUIRED, 'The time limit in seconds the worker can handle new messages'),
Copy link
Member

@nicolas-grekas nicolas-grekas Sep 25, 2020

Choose a reason for hiding this comment

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

not sure I see why this is better sorry

Copy link
Contributor Author

@sylfabre sylfabre Sep 25, 2020

Choose a reason for hiding this comment

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

@nicolas-grekas "The time limit in seconds the worker can run" = "the worker cannot work more than this time limit"
So this sentence is wrong because the worker can actually run longer than the time limit.

Mine is right but surely not the best one, feel free to propose something if you have better ideas.

Copy link
Member

@nicolas-grekas nicolas-grekas Sep 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I don't get why this tells something different. To me this is similar. Maybe it's my English...

Copy link
Contributor

@Guikingone Guikingone Sep 25, 2020

Choose a reason for hiding this comment

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

What about The time limit in seconds the worker can consume new messages?

Copy link
Contributor Author

@sylfabre sylfabre Sep 25, 2020

Choose a reason for hiding this comment

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

@nicolas-grekas the difference is

  • current sentence => when time's up, drop right now what you're doing and exit
  • proposed one => when time's up, take all the time you need to finish what you're doing, then exit and don't get/retrieve/consume/handle any other message

Copy link
Contributor Author

@sylfabre sylfabre Sep 25, 2020

Choose a reason for hiding this comment

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

@Guikingone thank you, I'll update

@sylfabre
Copy link
Contributor Author

sylfabre commented Sep 25, 2020 •
edited
Loading

@nicolas-grekas I'll explain why I'm working on this PR so you get the context.

I've found out this issue can happen:

  1. Send a message to a transport, expected to take a total of 3700 seconds to process (this is a design problem somewhere but keep this out of the current discussion)
  2. As per the doc, by default, the message will be redelivered after 3600 if not acknowledged
  3. Run a first messenger:consume --time-limit=3599 command => Command A
  4. Command A picks the message
  5. 1000 seconds later, run a second messenger:consume --time-limit=3599 command => Command B
  6. Command B waits as no messages are available
  • Expected behavior while reading the current state of the doc: command A exists after 3599 seconds, the message gets visible again and is now handled by command B: so at any time, only one command handles the message => no concurrency
  • Actual behavior while diving in the current code: command A keeps running the message after 3599 seconds, the message gets visible again so it is also handled by command B: so both commands now handle the same message at the same time.

I'm no expert with documentation but I've been misled by the current state of the documentation so I'm willing to put some effort to fix it and I appreciate your help to do so.

nicolas-grekas
nicolas-grekas approved these changes Sep 25, 2020
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

ok, works for me

@fabpot
Copy link
Member

fabpot commented Sep 26, 2020

Thank you @sylfabre.

@fabpot fabpot merged commit 013bbcc into symfony:master Sep 26, 2020
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
@sylfabre sylfabre deleted the messenger_doc branch June 2, 2021 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@sroze sroze Awaiting requested review from sroze

+1 more reviewer

@Guikingone Guikingone Guikingone left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Messenger Status: Reviewed

Projects

None yet

Milestone

5.2

Development

Successfully merging this pull request may close these issues.

5 participants

@sylfabre @fabpot @nicolas-grekas @Guikingone @carsonbot