Enable automatic message visibility refresh #87
Conversation
| while refresh_monitor | ||
| begin | ||
| # Extend the visibility timeout to the provided refresh timeout | ||
| message.change_visibility({ visibility_timeout: @visibility_refresh }) |
There was a problem hiding this comment.
what if the visibility_timeout is already set to the max limit of 12 hours?
There was a problem hiding this comment.
If this is lower, what is submitted to change_visibility will be respected first.
There was a problem hiding this comment.
I would expect this to throw an error since 12 hours is the max. We can't extend past that.
Additionally, I'm thinking of this scenario:
visiblity_timeoutis 2 hours,visbility_refreshis 1 hour- we receive a message, and start processing.
visibility_timeoutis extended to 3 hours - sleep for 30 minutes
- 30 minutes elapses (30 mins total),
visibility_timeoutis extended to 4 hours - 30 minutes elapses (1 hour total),
visibility_timeoutis extended to 5 hours - ...
In the above scenario, won't we hit the limit much faster than the time the job has to run? Basically what I'm trying to say is that we should cover the case where the visiblity_timeout has already maxed out.
| # Extend the visibility timeout to the provided refresh timeout | ||
| message.change_visibility({ visibility_timeout: @visibility_refresh }) | ||
| # Wait half the refresh timeout, and repeat | ||
| sleep(@visibility_refresh / 2.0) |
There was a problem hiding this comment.
trying to understand the use case of where a message completes but this thread is still sleeping. Will the worker start on the next message or will it be stalled because this thread is sleeping?
If the worker starts on the next message, will that message have a new thread for @monitor set up? Is there a case where consequent jobs finish quickly, and the monitor just has a bunch of sleeping threads and could eventually run out of threads to monitor the incoming messages
There was a problem hiding this comment.
also, does the @monitor thread need to have shutdown logic and be added to https://github.com/aws/aws-sdk-rails/blob/main/lib/aws/rails/sqs_active_job/poller.rb#L62-L64?
There was a problem hiding this comment.
Nah, as soon as the parent thread exits refresh_monitor will be set to false so this one will shut down. If a new job is picked up a new thread will be spun.
There was a problem hiding this comment.
Nah, as soon as the parent thread exits refresh_monitor will be set to false so this one will shut down
but when will it shut down? if this thread is sleeping, it could be shut down way after the parent thread finishes
|
Thanks for making updates. It looks like tests are failing, likely syntax. |
mullermp
added
the
pr/blocked
mullermp
added
pr/work-in-progress
Description of changes:
This PR will enable the auto-refresh of the visibility timeout of active messages.
This will allow users to use much lower default visibility timeouts, which will ensure messages can be returned to the queue promptly after a job fails, etc.
NOTE: I didn't do the work of adding automated tests yet, will do so if the work is acceptable.
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.