Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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.

# for free to join this conversation on GitHub. Already have an account? # 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