rebus-org / Rebus.SqlServer

:bus: Microsoft SQL Server transport and persistence for Rebus
https://mookid.dk/category/rebus
Other
43 stars 42 forks source link

Queue Ordering #32

Closed MrMDavidson closed 5 years ago

MrMDavidson commented 5 years ago

We noticed recently that the queries used, both for the leased and regular, SQL transport both filter out not-yet-visible messages (good) but then ignore the visibility value when performing ordering and instead only use the insertion order (See: https://github.com/rebus-org/Rebus.SqlServer/blob/master/Rebus.SqlServer/SqlServer/Transport/SqlServerTransport.cs#L308 ). If you imagine a snapshot of the queue that looks like (note: SentAt isn't a real column... it's just for illustration purposes. But imagine it's the time at which the message was inserted into the queue)

Id Visible Priority SendAt
1000 10:00 0 8:00
1500 9:30 0 8:59
2000 9:00 0 9:00

If we imagine the system has been under heavy load and is now, at 10:05, only just getting to this point in the queue the messages will be processed as 1000, 1500, and finally 2000. However the Visible property implies that the messages will be processed as (2000, 1500, 1000). This is because the current ordering is (Id, Priority). However I think it should actually be (Visible, Priority, Id).

Any thoughts here? We're not expecting, or relying on, strict queue ordering however we are seeing issues under heavy load where long-deferred messages are jumping ahead of messages, with the same priority, that under low load levels would have been processed long ago. Eg. If we were consuming messages faster than they would be produced this queue would have been processed as (2000, 1500, 1000).

mookid8000 commented 5 years ago

Small clarification: The current ordering is (Priority DESC, Id ASC), meaning that higher properties take precedence, and other than that messages are ordered by their insertion time.

So.... just to be entirely clear about this (my brain is struggling to work correctly these days, probably because of the heat πŸ”₯): What you're proposing is this: Change the ordering to (Visible ASC, Priority DESC, Id ASC)... but wouldn't that screw up the purpose of the priority column?

What if the ordering was (Priority DESC, Visible ASC, Id ASC) – wouldn't that mean that a higher priority always takes precedence, and then next comes the times at which the messages became visible – and only then then comes the ID (which will then only become relevant when messages are inserted within the same 100 ns interval, because that is the default precision of the datetimeoffset SQL type).

I think that would make the most sense – what do you think?

MrMDavidson commented 5 years ago

Yes, you're right. I mentally filtered out the Priority column as it wasn't affecting our example and then jammed it back in, incorrectly, when writing this issue. So, yes, my proposal would definitely screw up the use of the priority which wasn't my intent.

(Priority DESC, Visible ASC, ID ASC) sounds right to me. My only two questions would be;

  1. Do all messages get inserted with a Visible column set to a non-null value? If so, this definitely sounds right and like you said the Id column becomes mostly irrelevant.
  2. How would the other queue transports behave in this scenario? I'm not familiar enough with them to know but I'd imagine you'd want to keep approximate parity between them. Would this proposal break that parity?
mookid8000 commented 5 years ago

It'll be out in Rebus.SqlServer 5.0.0-b8 in a few minutes – thanks for pointing this out, I think it makes a lot of sense! πŸ‘

mookid8000 commented 5 years ago

Do all messages get inserted with a Visible column set to a non-null value? If so, this definitely sounds right and like you said the Id column becomes mostly irrelevant.

Yes – it simply uses getdate() and then adds whatever delay necessary (0 by default)

How would the other queue transports behave in this scenario? I'm not familiar enough with them to know but I'd imagine you'd want to keep approximate parity between them. Would this proposal break that parity?

Yes, thanks for reminding me – I'll go through both of them πŸ˜„