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

Rdb save incremental fsync #495

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

Conversation

thejambavan
Copy link

Pull Request (PR) description

minor change to the redis.conf EPP template to make it possible to disable rdb_save_incremental_fsync

This Pull Request (PR) fixes the following issues

Fixes #494

Copy link
Member

@kenyon kenyon left a comment

Choose a reason for hiding this comment

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

I don't see how this changes anything. If you are providing a non-undef value to redis::rdb_save_incremental_fsync, then it will render either rdb-save-incremental-fsync yes or rdb-save-incremental-fsync no.

To be sure, you could add a test like

describe 'test rdb-save-incremental-fsync for redis6' do
let(:params) do
{
rdb_save_incremental_fsync: true,
}
end
it { is_expected.to contain_file(config_file_orig).with('content' => %r{^rdb-save-incremental-fsync yes$}) }
end
that sets rdb_save_incremental_fsync to false and checks that the config files contains rdb-save-incremental-fsync no.

Copy link
Member

@kenyon kenyon left a comment

Choose a reason for hiding this comment

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

Actually I see the problem now. The if $rdb_save_incremental_fsync evaluates to false when you set rdb_save_incremental_fsync to false, so the line doesn't get rendered. But your patch doesn't fix that bug.

@kenyon kenyon added needs-tests needs-work not ready to merge just yet labels Nov 9, 2023
@thejambavan
Copy link
Author

thejambavan commented Nov 10, 2023

I have corrected the if to an unless Undef and have added tests which seem to do the job (apologies for multiple formatting commits) for all three states:

Undef: no config file entry, aka "the default" which may have been "no" in the past but is now "yes"
true: yes
false: no

I have separately verified that this works as intended on a real-world instance.

edit: i clearly do not now how to use rspec properly

describe 'test rdb-save-incremental-fsync Undef for redis6' do
let(:params) do
{
rdb_save_incremental_fsync: Undef,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
rdb_save_incremental_fsync: Undef,
rdb_save_incremental_fsync: nil,

@thejambavan
Copy link
Author

no matter what i do with the test, when i set:

            rdb_save_incremental_fsync: nil,

as part of the rspec test, i can't get the test to pass in the Puppet/ruby stages. I don't see any other tests for undef/nil, and have not had any luck finding good examples elsewhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-tests needs-work not ready to merge just yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rdb_save_incremental_fsync cannot be turned off
2 participants