php-enqueue / enqueue-dev

Message Queue, Job Queue, Broadcasting, WebSockets packages for PHP, Symfony, Laravel, Magento. DEVELOPMENT REPOSITORY - provided by Forma-Pro
https://enqueue.forma-pro.com/
MIT License
2.17k stars 434 forks source link

Fix #1257 #1258

Closed andrewprofile closed 2 years ago

makasim commented 2 years ago

I am not sure I follow it. Can you please walk me through your thought process?

andrewprofile commented 2 years ago

@makasim I described the problem in issue #1257 :)

microtime return value in seconds so the conversion to timestamp is invalid, ex : 16551970635236 -> GMT: Monday, 5 July 2494 17:37:15.236

Steveb-p commented 2 years ago

@makasim I described the problem in issue #1257 :)

microtime return value in seconds so the conversion to timestamp is invalid, ex : 16551970635236 -> GMT: Monday, 5 July 2494 17:37:15.236

The issue here is that without multiplication - that you're removing - precision of stored data suffers. Messages become stored with at most 1 second precision, which makes it impossible to maintain their order & microtime() becomes equivalent to time().

I think the issue you're describing is actually valid, but not because of the code. It's because we've called this property "timestamp" while we intended to store a more precise value.

a.k.a. naming is hard :sweat_smile:

EDIT: Actually, where did you find that this value is supposed to be a timestamp? The class that this code is contained with specificly uses DbalType::INTEGER (which, btw, may not be long enough @makasim and a conversion to BIGINT might be considered)

andrewprofile commented 2 years ago

@Steveb-p Sorry it's my mistake. The property published_at suggests that it will be the date, but since you operate everywhere on timestamps, I assumed that in this case also to be able to get a specific date from it. But I already understand that this is not the point, but then there is some inconsistency because other values do not operate in such precision.

Closing

Steveb-p commented 2 years ago

@Steveb-p Sorry it's my mistake. The property published_at suggests that it will be the date, but since you operate everywhere on timestamps, I assumed that in this case also to be able to get a specific date from it. But I already understand that this is not the point, but then there is some inconsistency because other values do not operate in such precision.

If you find any discrepancy in this or other transports feel free to open an issue or discussion (which, btw, could be enabled @makasim, so issues don't get polluted by questions). I would not outright dismiss the notion that there might be differences :sweat_smile:

makasim commented 2 years ago

@Steveb-p Enabled discussions https://github.com/php-enqueue/enqueue-dev/discussions

andrewprofile commented 2 years ago

@Steveb-p I meant redeliverAfter or timeToLive properties, that's why I pointed it out and assumed it was an issue :)