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.

Sign up for GitHub

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

Jump to bottom

Rdb save incremental fsync #495

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

Rdb save incremental fsync #495

wants to merge 29 commits into 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

kenyon
kenyon reviewed Nov 9, 2023
View reviewed changes
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

puppet-redis/spec/classes/redis_spec.rb

Lines 1683 to 1691 in e8d71af

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.

kenyon
kenyon requested changes Nov 9, 2023
View reviewed changes
Copy link
Member

@kenyon kenyon left a comment •
edited
Loading

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 •
edited
Loading

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

kenyon
kenyon reviewed Nov 10, 2023
View reviewed changes
spec/classes/redis_spec.rb Outdated
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
minot commit bump to trigger CI
98b7504
@thejambavan
Undef -> nil in spec
c4e308e
@thejambavan
Merge branch 'master' into rdb-save-incremental-fsync
5cbb8fe
@thejambavan
attempt test for negation using regex rather than as i am less famili…
757e9d9 
…ar with its behaviour
@thejambavan
perhaps i want instead
440a70c
@thejambavan
maybe i made an infocorrect assumption in the EPP template
5e2f74b
@thejambavan
sometimes it can be a string
c5b865a
@thejambavan
reorder tests to test undef/nil variant first
442dfdc
@thejambavan
reorder tests to test undef/nil variant first
01d5160
@thejambavan
reorder tests to test undef/nil variant first
976ad0a
@thejambavan
misspelled as rdb_save_incremental_fsync
52fe4a7
@thejambavan
easier to test for a commented-out line
2fe204b
@thejambavan
easier to test for a commented-out line
cd91071
@thejambavan
easier to test for a commented-out line
0fa6900
@thejambavan
i give up on this test. It works in reality, I could really do with a…
00a1d33 
… hand 🤷
@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
Reviewers

@kenyon kenyon kenyon requested changes

Assignees
No one assigned
Labels
needs-tests needs-work not ready to merge just yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

rdb_save_incremental_fsync cannot be turned off
2 participants
@thejambavan @kenyon