Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Limit rabbitmq prefetch and use default rabbitmq #180

Merged
merged 3 commits into from
Dec 13, 2023

Conversation

stefpiatek
Copy link
Contributor

Shouldn't need to increase the timeout for rabbitmq so reverting to pulling the image directly.

Testing these changes from the debug branch worked 🎉. Have started a new test run in /gae/pixl_dev to make sure that it doesn't disconnect overnight

Closes #143
Closes https://github.com/UCLH-Foundry/the-rolling-skeleton/issues/68

Shouldn't need to increase the timeout for rabbitmq
jeremyestein
jeremyestein previously approved these changes Dec 13, 2023
Copy link
Contributor

@jeremyestein jeremyestein left a comment

Choose a reason for hiding this comment

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

Looks good, nice simple fix :)
It's worth noting that given our 2 minute processing timeout, this leaves unacked messages of max 20 minutes old, which is still below the rabbitmq ack timeout threshold of 30 minutes. However since we hit that 2 minute timeout quite often we may have to raise it, which might necessitate lowering the prefetch_count further.
I don't think we're gaining any notable performance from prefetching due to the long processing time for each message, so arguably 1 is a more robust value.

@stefpiatek
Copy link
Contributor Author

Yeah that's a fair point, I'll set to 1 and we can readdress if we find a reason to change it

@stefpiatek stefpiatek merged commit 08ac69c into main Dec 13, 2023
7 checks passed
@stefpiatek stefpiatek deleted the stef/143-limit-prefetch branch December 13, 2023 12:24
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.

RabbitMQ disconnects from PACS queue
2 participants