Modify drain_all to respect enqueued order

[ggilder]

Currently Sidekiq::Worker.drain_all effectively reorders the queue by
grouping job classes together. This change maintains the enqueued order
of jobs in the queue when using drain_all.

[mperham]

It is a smell to depend on execution ordering in an async system.

[ggilder]

I agree in principle, but you already have deterministic ordering here — it's just not the natural ordering that jobs were enqueued in.

[mperham]

Good point, we should make it random. :-D

…

On Apr 3, 2017, at 18:25, Gabriel Gilder ***@***.***> wrote: I agree in principle, but you already have deterministic ordering here — it's just not the natural ordering that jobs were enqueued in. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[ggilder]

Haha... ok, so maybe to make this more concrete I can present the problem that led me to stumble on this.

I have an app where I use Cucumber scenarios to test complex flows. They make fairly heavy use of tables to concisely document the state of the database before and after certain actions are performed, so something like this simplified example:

Then expect the incidents table to contain:
  | id  | source | case_id | state       | amount |
  | 801 | abc01  | 10001   | active_case | 4000   |
  | 802 | abc01  | 10002   | active_case | 1600   |
  | 803 | abc02  | 10003   | active_case | 1600   |
  | 804 | abc02  | 10004   | active_case | 2300   |

So there's an implicit assumption that associated models (in this example, cases and incidents) are created in order because we enqueue jobs that process each incident in order.

Now, you could certainly rewrite that to not specify the foreign key ids, but it would be much more cumbersome. So from my perspective, maintaining the queue order is a convenience that allows my test scenarios to be simpler and easier to maintain.

If this is not something you want to support in sidekiq, that's fine... just thought I'd give some context in case that sways you at all. :)

[mperham]

You can assert the ID is non-nil, you can assert it is within a range but I still maintain it's a bad idea to depend on precise code ordering within your integration tests unless you explicitly execute the code yourself.