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

pipeline doesn't respect (at least) first message delay option #206

Closed
metheoryt opened this issue Apr 10, 2019 · 1 comment · Fixed by #264
Closed

pipeline doesn't respect (at least) first message delay option #206

metheoryt opened this issue Apr 10, 2019 · 1 comment · Fixed by #264
Labels
beginner friendly Enhancements or issues that are easy for someone to get started on. enhancement
Milestone

Comments

@metheoryt
Copy link
Contributor

if i create pipeline as below and try to run it:

pipe = actor.message_with_options(delay=60_000) | actor.message()
pipe.run()

the pipe will be executed immediately though.
I think, pipeline should respect message delays, so e.g. if both first message delay and pipe depay specified, they should summarize.

@Bogdanp Bogdanp added beginner friendly Enhancements or issues that are easy for someone to get started on. enhancement labels Apr 11, 2019
@Bogdanp Bogdanp added this to the v1.7.0 milestone Apr 11, 2019
@synweap15
Copy link
Contributor

synweap15 commented Jan 13, 2020

I think currently only the pipe.run(delay=60_000) invocation will take effect, the delay parameter of messages will be ignored:

  1. For the first message in a pipe, the run method named argument is taken, the message's one isn't considered:

    self.broker.enqueue(self.messages[0], delay=delay)

  2. For next-in-line messages, there's no delay taken into consideration:

    broker.enqueue(next_message)

I've got test cases and a fix ready, however in contrast to what @metheoryt proposes, I think it would be best to follow this logic:

  1. If there's either a delay specified for the first message or there's a delay specified in pipe run invocation, take the one that's present
  2. If they are both present, take the one that's bigger (instead of adding them).

And of course, the messages after the first message in a pipe should have their delays respected as well.

The delay parameter of a pipeline is specified as The minimum amount of time, in milliseconds, the pipeline should be delayed by., so I think taking the bigger value of the pipeline's delay and the first message's delay would be reasonable. What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginner friendly Enhancements or issues that are easy for someone to get started on. enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants