Shuffle glob queues per env flag #1586
Conversation
ermanc
force-pushed
the
shuffle_glob_queues_per_env_flag
branch
from
1a58503
to
7a2a151
Compare
|
|
||
| # Queues should not have been processed in sorted order | ||
| assert all_queues.sort != processed_queues | ||
| end |
There was a problem hiding this comment.
This is cool. My only concern is that the test is quite complex. Since the guts of the change is using shuffle instead of sort how about trying to test #glob_match in isolation instead? Is that possible?
There was a problem hiding this comment.
It's possible but I personally prefer the current spec as it demonstrates the purpose and usage like the other specs here. It's also very loosely coupled to implementation.
|
Ok, I’m not sure I agree but fair enough. I think a unit test is enough to
cover what you’re doing; setting an ENV variable and then checking that a
method has been called. You could always demonstrate the functionality with
documentation in the README. Regardless the change is interesting :)
…On Mon 23 Oct 2017 at 19:38, Erman Celen ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In test/worker_test.rb
<#1586 (comment)>:
> + new_queues.each { |q| Resque::Job.create(q.to_sym, GoodJob) }
+ all_queues = ['jobs'] + new_queues
+
+ processed_queues = []
+ @worker = Resque::Worker.new("*")
+ without_forking do
+ with_shuffling do
+ @worker.work(0) do |job|
+ processed_queues << job.queue
+ end
+ end
+ end
+
+ # Queues should not have been processed in sorted order
+ assert all_queues.sort != processed_queues
+ end
It's possible but I personally prefer the current spec as it demonstrates
the purpose and usage like the other specs here. It's also very loosely
coupled to implementation.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1586 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAMuf-rjzmb5stVEextNPTsJxwfL8m29ks5svN01gaJpZM4P_lPZ>
.
|
| end | ||
|
|
||
| # Queues should not have been processed in sorted order | ||
| assert all_queues.sort != processed_queues |
There was a problem hiding this comment.
Even that it is small I think there is a chance that the shuffled array will be the same as the sorted array right? Would not this test be broken in that case?
There was a problem hiding this comment.
There is a chance the shuffled array will be the same as the sorted array, yes. I'm more confident in a test case that tests the observable effect rather than implementation (e.g. spying on the sort vs shuffle call) and I cannot think of another way to test the "shuffled" outcome other than it being not sorted.
I certainly welcome any suggestions on a better testing strategy, as I find this an interesting behavior to test for.
There was a problem hiding this comment.
Yeah. I think the comment with the probability is good enough for that.
| end.sort | ||
| end | ||
|
|
||
| if ENV['SHUFFLE'] && ENV['SHUFFLE'] != 'false' |
There was a problem hiding this comment.
Set up wildcard glob sort/shuffle behavior in #initialize
There was a problem hiding this comment.
What do think of making this a little more forgiving?
| if ENV['SHUFFLE'] && ENV['SHUFFLE'] != 'false' | |
| if ENV['SHUFFLE'] && ENV['SHUFFLE'].downcase != 'false' |
|
i am interested in this, for reasons explained in PR description at top. Is there any hope for it? Anything I can do to help bring it home? |
There was a problem hiding this comment.
This is a very old PR, but still a great change! Thanks for the contribution. I've slowly been going through old PRs since I've been added as a contributor.
@ermanc if there's still interest in getting this merged, could you:
- Rebase on master and make sure tests are passing
- Add some docs to the readme about how this works
Also, if someone else wants to pick this up (@jrochkind) that's great too!
| end.sort | ||
| end | ||
|
|
||
| if ENV['SHUFFLE'] && ENV['SHUFFLE'] != 'false' |
There was a problem hiding this comment.
What do think of making this a little more forgiving?
| if ENV['SHUFFLE'] && ENV['SHUFFLE'] != 'false' | |
| if ENV['SHUFFLE'] && ENV['SHUFFLE'].downcase != 'false' |
Allows developers the option to have shuffling
*queue processing viaENV['SHUFFLE'].The reasoning behind is two fold:
As mentioned by @steveklabnik in Shuffling queues on * #1075 (comment),
*processing kind of implies lack of explicit prioritization. If you really care about certain prioritization to be maintained, explicit queue lists are suited.Alphabetical prioritization means queues that are processed by general
*workers are more prone to having one obnoxious queue (on purpose or accidentally) effectively block others just because they are higher up alphabetically. Using shuffling, an obnoxious queue cannot block or even slow down other queues as all have statistically similar chances of being processed every turn.Usage is non-default, so the default behavior is still sorted
*queues.Feature is enabled via
SHUFFLE=true QUEUE=* rake resque:work