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

feat: add concurrent option to utube #76

Closed
wants to merge 1 commit into from

Conversation

aleclarson
Copy link
Contributor

@aleclarson aleclarson commented Feb 16, 2018

Inspired by #15, this commit adds a concurrent: number option to the utube queue type.

Additionally, you can now do :take({ utube = '...' }) to take from a specific sub-queue.

The default behavior of utube has not changed.
Only one task per utube sub-queue may run at any time.

Set concurrent to 2 and only two tasks per sub-queue may run at any time.
Set concurrent to 1/0 or math.huge and no throttling is enforced.

@aleclarson aleclarson force-pushed the concurrent branch 8 times, most recently from 08392b0 to 863f558 Compare February 16, 2018 21:59
Also adds support for taking from a specific sub-queue by passing
`{ utube = '...' }` to the `:take` method.
@aleclarson
Copy link
Contributor Author

aleclarson commented Feb 16, 2018

Alright, ready for a code review.

The is_throttled method uses the most efficient means of determining concurrency for the given sub-queue name.

When :take is called without a specific sub-queue name, we traverse all READY tasks, but make sure to avoid calling is_throttled for the same sub-queue more than once. I removed the GE iterator, because it didn't look like it was necessary. Correct me if I'm wrong there.

Edit: Actually, the is_throttled method could be changed to iterate until self.concurrent is reached, instead of using :count to count every single TAKEN task. This is only an issue if you set concurrent to anything other than 1 or 1/0 (or you leave it nil).

@LeonidVas
Copy link
Contributor

Hi. Thank you for the patch. Sorry for the big latency. Please, could you to describe few use case of the feature?

@aleclarson
Copy link
Contributor Author

It's been a while, but I guess being able to :take multiple tasks from a sub-queue at one time had a good enough use case for me to implement it. :)

Is there another way to achieve that already?

@LeonidVas
Copy link
Contributor

As work around: "You can use some manager around fifo/fifottl".
I agree that this may be a good ability, but without understanding the purpose of the ability, it is useless.

@aleclarson
Copy link
Contributor Author

To me, the purpose is obviously useful. There are times where you want to process multiple tasks from the same sub-queue at once. You can't do that without this PR. Not sure what more could be explained here. ¯\(ツ)

@LeonidVas LeonidVas self-requested a review October 19, 2020 11:28
@LeonidVas
Copy link
Contributor

After some discussion, we decided not add this feature to the queue. If the situation change, we get back to the PR.

@LeonidVas LeonidVas closed this Oct 20, 2020
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.

2 participants