Skip to content

Updated Retries middleware#629

Merged
Bogdanp merged 3 commits intoBogdanp:masterfrom
kuba-lilz:kuba/requeue_time
Jun 12, 2024
Merged

Updated Retries middleware#629
Bogdanp merged 3 commits intoBogdanp:masterfrom
kuba-lilz:kuba/requeue_time

Conversation

@kuba-lilz
Copy link
Contributor

For motivation see discussion at
https://groups.io/g/dramatiq-users/topic/how_to_check_how_long_message/106519634

Retries middleware now records requeue_timestamp on message options in before_enqueue function if message failed execution.

Hoped the implementation and tests are up to required standards, if not, let me know what needs to be changed.

It now records requeue_timestamp in message options in before_enqueue function if message failed execution
It now records and asserts all requeue timestamps
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you’re changing the Retries middleware directly, I would move this line under line 98 and avoid the check and separate before enqueue hook altogether.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My small concern was that maybe, just maybe, in corner cases there might be some lag between execution of that part of code and enqueue operation, given how dramatiq might be running workers doing heavy CPU operations at the same time.
I shouldn't worry about that?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the most likely place for this thread to relinquish control is going to be the io that is going to be done when actually enqeueuing the message so I think the chances of the suggested approach being less reliable for timing are very slim (it might in fact be more reliable because it’s doing less work).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Roger that, I'll refactor and push again 🫡

Moved logic for recording requeue_timestamp from before_enqueue to after_process_message
@Bogdanp
Copy link
Owner

Bogdanp commented Jun 12, 2024

Thanks!

@Bogdanp Bogdanp merged commit 7f8caac into Bogdanp:master Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments