Webador / SlmQueue

Laminas / Mezzio module that integrates with various queue management systems.
Other
137 stars 56 forks source link

Add MaxTimeStrategy incl. docs and tests #209

Closed MatthiasKuehneEllerhold closed 6 years ago

MatthiasKuehneEllerhold commented 6 years ago

This PR adds a new strategy for terminating workers after a set amount of time.

We need this functionality in order to deal with some external interfaces and their timeouts out of our control. We dont want to have 1 endless running worker but instead have workers that run for a set amount of time and respawn.

This strategy is off by default (in order to keep BC) and is totally optional. Reasonable default values (according to our interpretation) are set (1 hour timeout) and can be overwritten with the archicture thats already in place. We figured we're not the only ones needing this kind of strategy, what do you think?

Documentation and Unit-Tests are in this as well.

roelvanduijnhoven commented 6 years ago

Thanks for the PR @MatthiasKuehneEllerhold. Code looks great. I have two points that really both revolve around the name of the strategy.

Not sure what name would better reflect this. Curious to hear what you think @MatthiasKuehneEllerhold . What about StopWorkerAfterTimeStrategy?

The MaxRunsStrategy does not have this ambiguity as time is not really

May I ask what company you are using this repo for? Just curious :).

basz commented 6 years ago

LifetimeStrategy?

roelvanduijnhoven commented 6 years ago

WorkerLifetimeStrategy sounds good to me. @MatthiasKuehneEllerhold if you agree; could you adapt PR and I'll happily merge it! Thanks again.

MatthiasKuehneEllerhold commented 6 years ago

Happy New Year!

I've renamed the strategy and clarified its usage in the documentation.

My company should be visible in my profile ;)

roelvanduijnhoven commented 6 years ago

Happy new year indeed!

@MatthiasKuehneEllerhold The tests fail; think you forgot to rename something.

MatthiasKuehneEllerhold commented 6 years ago

You're right. Should be fixed now.

roelvanduijnhoven commented 6 years ago

Still test fail @MatthiasKuehneEllerhold. Please check results yourself before comment (and optionally test locally before pushing ;)).

MatthiasKuehneEllerhold commented 6 years ago

Sorry for that mishap (bad start for a new year :/). Tests are green now!

roelvanduijnhoven commented 6 years ago

Thanks! Will release soon in the 1.0.2. release.