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

Add helper to cleanup delayed sidekiq timechunks #894

Merged

Conversation

Drowze
Copy link
Contributor

@Drowze Drowze commented Sep 4, 2023

For context:

#869 added the delayed_sidekiq strategy, which by design will only create a sidekiq worker to update an index every N seconds (10 seconds by default).
The implementation details for that involves redis sorted sets: it creates a sorted set for each index that uses the strategy and adds a new member to it everytime we call reindex (with the weight being calculated from the current timestamp, generating a unique number every N seconds).

The problem:

Since we're writing directly to Redis, if we use this strategy in tests we're subject to state leaking and creating flaky tests. For example:

  • test A runs some code that calls MyClass.import!(1, strategy: :delayed_sidekiq) and not flush the Sidekiq workers or the redis database
  • in less than 10 seconds, test B runs some code that calls MyClass.import!(1, strategy: :delayed_sidekiq), flushes Sidekiq workers and expects the index to be updated.

On the above scenario, test B will fail as no worker will be enqueued since import! was called within 10 seconds in between the tests (also note that Sidekiq runs in memory by default when running tests, so workers enqueued by test A are not present in test B).

The fix:

This PR adds a way to workaround the issue by cleaning up the saved timechunks. It can easily be added to e.g. rspec hooks via:

RSpec.configure do |config|
  config.after { Chewy::Strategy::DelayedSidekiq.clear_timechunks! }
end

Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the changelog if the new code introduces user-observable changes. See changelog entry format for details.

@Drowze Drowze force-pushed the rg-delayed_sidekiq-cleanup-test-helper branch 2 times, most recently from caf9707 to 980012c Compare September 4, 2023 21:21
@konalegi
Copy link
Contributor

konalegi commented Dec 9, 2023

Could you please rebase PR?

@konalegi
Copy link
Contributor

hey @Drowze , could you please rebase your PR?

@Drowze Drowze force-pushed the rg-delayed_sidekiq-cleanup-test-helper branch from 980012c to 5f7bf51 Compare December 16, 2023 11:51
@Drowze
Copy link
Contributor Author

Drowze commented Dec 16, 2023

Hey @konalegi, sorry for the delay! I didn't see the notification for the first message 😅

Just rebased this PR! 👍

@konalegi
Copy link
Contributor

Hey, unfortunately rubocop is failing :(
I suppose it's because we recently switched from ruby 2.x to 3.x syntax.

Mainly useful to avoid flaky tests when using a real redis database
and not flushing the database in between tests
@Drowze Drowze force-pushed the rg-delayed_sidekiq-cleanup-test-helper branch from 5f7bf51 to c0b4b66 Compare December 16, 2023 16:41
@Drowze
Copy link
Contributor Author

Drowze commented Dec 16, 2023

I see! Just fixed the rubocop issues and updated the PR 😄

@konalegi konalegi merged commit ea0ed0a into toptal:master Dec 16, 2023
10 checks passed
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