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

Warn on boot if maxmemory policy is not 'noeviction' #4752

Merged
merged 1 commit into from Dec 2, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions lib/sidekiq/cli.rb
Expand Up @@ -62,6 +62,11 @@ def run(boot_app: true)
ver = Sidekiq.redis_info["redis_version"]
raise "You are connecting to Redis v#{ver}, Sidekiq requires Redis v4.0.0 or greater" if ver < "4"

maxmemory_policy = Sidekiq.redis_info["maxmemory_policy"]
if maxmemory_policy != "noeviction"
Copy link
Collaborator

Choose a reason for hiding this comment

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

There will be a lot of these in development. I'd suggest you only print this if env != "development".

Copy link
Contributor Author

@odlp odlp Dec 2, 2020

Choose a reason for hiding this comment

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

Taking a look at redis.conf in various versions, I believe noeviction is the default:

But appreciate that it's a somewhat meaningless check in development, apart from the educational value. I'll add the additional condition 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch, if it’s the default in the brew formula then I’m ok with it as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

logger.warn "'noeviction' maxmemory policy is recommended (current policy: '#{maxmemory_policy}'). See: https://github.com/mperham/sidekiq/wiki/Using-Redis#memory"
end

# Since the user can pass us a connection pool explicitly in the initializer, we
# need to verify the size is large enough or else Sidekiq's performance is dramatically slowed.
cursize = Sidekiq.redis_pool.size
Expand Down
26 changes: 26 additions & 0 deletions test/test_cli.rb
Expand Up @@ -481,6 +481,32 @@ def logdev
assert_includes @logdev.string, "Booted Rails #{::Rails.version} application in production environment"
end
end

describe 'checking maxmemory policy' do
it 'warns if the policy is not noeviction' do
redis_info = { "maxmemory_policy" => "allkeys-lru", "redis_version" => "6" }

Sidekiq.stub(:redis_info, redis_info) do
subject.stub(:launch, nil) do
subject.run
end
end

assert_includes @logdev.string, "'noeviction' maxmemory policy is recommended (current policy: 'allkeys-lru')"
end

it 'silent if the policy is noeviction' do
redis_info = { "maxmemory_policy" => "noeviction", "redis_version" => "6" }

Sidekiq.stub(:redis_info, redis_info) do
subject.stub(:launch, nil) do
subject.run
end
end

refute_includes @logdev.string, "'noeviction' maxmemory policy is recommended"
end
end
end

describe 'signal handling' do
Expand Down