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

Introduce poll-interval-variance option #431

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tomgi
Copy link
Contributor

@tomgi tomgi commented Sep 20, 2024

Enable this for cleaner diff:
image


Context

We run multiple ECS tasks of que that are all started almost exactly at the same time during every deployment.
That means that they also start polling at the same time, and continue doing so in such a synchronized fashion.

More specifically, in our case:

  • we run 4 que processes
  • poll-interval is set to 20s

Meaning that on average we poll every 5s, but in reality, we poll 4 times every 20s 🙂

We would like to introduce a random variance to the polling interval, so that the polling is more evenly distributed.

Changes

Add optional poll-interval-variance CLI parameter defaulting to 0.0.

It is used to make each individual poll time slightly randomised within the range of poll-interval +/- poll-interval-variance.

This way tasks started at the same time won't continue always polling at the same time.

Backward-compatibility

When the new poll-interval-variance parameter is not provided, there's no change compared to the current behaviour.

@@ -275,7 +275,7 @@ def assert_poll(priorities:, locked:)
assert_equal false, poller.should_poll?
end

it "should be false if the number of jobs returned from the last poll was less than the lowest priority request, but the poll_interval has elapsed" do
it "should be true if the number of jobs returned from the last poll was less than the lowest priority request, but the poll_interval has elapsed" do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was a typo, the test is actually checking assert_equal true, which is the expected behavior.

@tomgi tomgi marked this pull request as ready for review September 23, 2024 01:31
job_ids = 5.times.map { Que::Job.enqueue.que_attrs[:id] }

result = poller.poll(priorities: { 500 => 7 }, held_locks: Set.new)
assert_equal job_ids, result.map(&:id)

poller.instance_variable_set(:@last_polled_at, Time.now - 30)
assert_equal true, poller.should_poll?
Timecop.freeze(Time.now + 30) do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change makes the spec less coupled with implementation details.

Instead of setting private instance variables, it 'time travels' 30 seconds into the future, after the poll_interval has already elapsed.

@tomgi tomgi force-pushed the poll-interval-variance branch from c23171b to 5e50c41 Compare October 28, 2024 04:33
@tomgi tomgi force-pushed the poll-interval-variance branch from 5e50c41 to 6ccd8bc Compare October 28, 2024 04:36
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.

1 participant