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

[fix] delayed_sidekiq race condition #937

Merged
merged 1 commit into from
May 3, 2024

Conversation

skcc321
Copy link
Contributor

@skcc321 skcc321 commented Apr 10, 2024

In the PR I'm addressing two issues with delayed_sidekiq strategy.

  1. There is a chance for race conditions exactly in this place.
            unless redis.zrank(timechunks_key, timechunk_key) # read
              redis.zadd(timechunks_key, at, timechunk_key) # write

I eliminated it using Lua script and making the combination of operations atomic.

  1. Also, there was a possibility to bloat Redis if there is an issue with reindexing (ES connection timeout for instance)
    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.

@skcc321 skcc321 force-pushed the master-safe-delayed-sidekiq branch from e813b78 to cf58036 Compare April 10, 2024 21:26
Copy link

@konalegi konalegi left a comment

Choose a reason for hiding this comment

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

:wow: great!

@konalegi
Copy link

please ping me once you are ready to merge. I'll release it

@konalegi
Copy link

Please fix rubocop issues, and something is odd with our specs and CI in general :/

@konalegi
Copy link

Please take a look at your PR to see what's wrong. I created a separate PR to check CI and it seems to be working. #942

@ebeagusamuel
Copy link
Contributor

ebeagusamuel commented Apr 23, 2024

@skcc321 Please try rebasing on toptal/chewy master branch.

@skcc321 skcc321 force-pushed the master-safe-delayed-sidekiq branch from 9bb8bd7 to a599453 Compare April 26, 2024 07:31
@skcc321
Copy link
Contributor Author

skcc321 commented Apr 26, 2024

Sorry for the delay, guys.
Yep, I rebased it on upstream/master today.
So, we've been testing it on prod 2 weeks. Looks like it completely resolved the race condition issue.
@konalegi, @ebeagusamuel should be ready to merge now. Could you guys restart the CI to verify the PR?

@skcc321 skcc321 force-pushed the master-safe-delayed-sidekiq branch from a599453 to f6e19b8 Compare April 26, 2024 07:35
@@ -2,17 +2,18 @@

if defined?(Sidekiq)
require 'sidekiq/testing'
require 'mock_redis'
Copy link

@konalegi konalegi Apr 30, 2024

Choose a reason for hiding this comment

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

Do we need mock_redis gem after that?

@konalegi
Copy link

Since you've started using real redis in specs, you have to redis service, smth like and provide proper env variables for sidekiq/redis depending how you want to test it.

jobs:
  ruby-3:
    services:    
      redis:
        # Docker Hub image
        image: redis
        # Set health checks to wait until redis has started
        options: >-
          --health-cmd "redis-cli ping"
          --health-interval 10s
          --health-timeout 5s
          --health-retries 5

@skcc321 skcc321 force-pushed the master-safe-delayed-sidekiq branch from f6e19b8 to d3f3081 Compare May 2, 2024 12:08
@skcc321 skcc321 requested a review from a team as a code owner May 2, 2024 12:08
@skcc321
Copy link
Contributor Author

skcc321 commented May 2, 2024

Since you've started using real redis in specs, you have to redis service, smth like and provide proper env variables for sidekiq/redis depending how you want to test it.

jobs:
  ruby-3:
    services:    
      redis:
        # Docker Hub image
        image: redis
        # Set health checks to wait until redis has started
        options: >-
          --health-cmd "redis-cli ping"
          --health-interval 10s
          --health-timeout 5s
          --health-retries 5

done. let's try again the CI run.

@konalegi
Copy link

konalegi commented May 2, 2024

@skcc321 still failing :/

@skcc321
Copy link
Contributor Author

skcc321 commented May 2, 2024

@skcc321 still failing :/

OK, I'll take a look tonight.

@skcc321 skcc321 force-pushed the master-safe-delayed-sidekiq branch 2 times, most recently from 649dc08 to c07d05a Compare May 2, 2024 20:18
@skcc321
Copy link
Contributor Author

skcc321 commented May 2, 2024

@konalegi one more attempt please

@skcc321 skcc321 force-pushed the master-safe-delayed-sidekiq branch from c07d05a to 7b546c1 Compare May 3, 2024 07:09
@skcc321 skcc321 force-pushed the master-safe-delayed-sidekiq branch from 7b546c1 to 784fe38 Compare May 3, 2024 07:35
@skcc321
Copy link
Contributor Author

skcc321 commented May 3, 2024

@konalegi looks like two failed checks are not related to the changes I made.
could you restart them?

@skcc321
Copy link
Contributor Author

skcc321 commented May 3, 2024

@konalegi one of them has passed. Could you restart the last one?

@konalegi
Copy link

konalegi commented May 3, 2024

Great, merging!

@konalegi konalegi merged commit c99741e into toptal:master May 3, 2024
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.

5 participants