search

rebus-org / Rebus.SqlServer

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

Timestamp handling improvements #57

Closed tompazourek closed 5 years ago

tompazourek commented 5 years ago

Rebus is MIT-licensed. The code submitted in this pull request needs to carry the MIT license too. By leaving this text in, I hereby acknowledge that the code submitted in the pull request has the MIT license and can be merged with the Rebus codebase.

The changes I made in the PR:

(1.) Changed from GETDATE to SYSDATETIMEOFFSET.

GETDATE is the incorrect function to be used with values that have datetimeoffset type (as do the columns in the transport tables). GETDATE returns a value of datetime which gets converted by SQL Server into datetimeoffset. But by doing so, it loses information about the timezone. This means that the timestamps stored will end up incorrect (they'll be stored with the UTC offset, even though they GETDATE returns local offset time).

(2.) Improved the precision of visibility delay and TTL from seconds to milliseconds.

With the previous second-level precision, it sometimes happened that the deferred messages were dispatched almost a second earlier than they should be. With the millisecond precision, the deferred messages get dispatched more accurately.

During the implementation of this, I noticed that the DATEADD function has a limit in the number parameter, which is the maximum value of INT (2147483647). With seconds, we can add up to 68.05 years of range. With milliseconds, this would be only 24.86 days. So I decided to pass the time span as two INT values TotalSeconds (ranging from 0 to 2147483647) and Milliseconds (ranging from 0 to 999). This way, we're able to overcome this limitation.

(3.) Updated the time span handling in SqlServerLeaseTransport to match the handling in SqlServerTransport.

The SqlServerLeaseTransport used to pass BIGINT values into the DATEADD function, but DATEADD only actually supports the range of INT. I change the handling so that it will pass two separate INT parameters (one for total seconds, one for 0-999 milliseconds) to overcome this limitation. I think this will work better and it's now the same approach that I used in SqlServerTransport, which makes it more uniform.

Each of these changes was made in a separate commit.

I can explain the reasoning behind some of the changes in more detail if it's not clear, or change something if you're not happy with the code.

Some related information is in here: https://github.com/rebus-org/Rebus.SqlServer/issues/48#issuecomment-513179548

CLAassistant commented 5 years ago

CLA assistant check
All committers have signed the CLA.

mookid8000 commented 5 years ago

Awesome! It all makes sense 😁

mookid8000 commented 5 years ago

I've pushed a new version with these changes now. It's out as Rebus.SqlServer 6.0.0-a9