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

Always create base AOF file when redis start from empty. #10102

Merged
merged 14 commits into from
Jan 13, 2022
Merged

Always create base AOF file when redis start from empty. #10102

merged 14 commits into from
Jan 13, 2022

Conversation

chenyang8094
Copy link
Collaborator

@chenyang8094 chenyang8094 commented Jan 11, 2022

See issue #9794

Force create a BASE file (use a foreground rewriteAppendOnlyFile) when redis starts from an empty data set and appendonly is yes.

The reasoning is that normally, after redis is running for some time, and the AOF has gone though a few rewrites, there's always a base rdb file. and the scenario where the base file is missing, is kinda rare (happens only at empty startup), so this change normalizes it.
But more importantly, there are or could be some complex modules that are started with some configuration, when they create persistence they write that configuration to RDB AUX fields, so that can can always know with which configuration the persistence file they're loading was created (could be critical). there is (was) one scenario in which they could load their persisted data, and that configuration was missing, and this change fixes it.

Add a new module event: REDISMODULE_SUBEVENT_PERSISTENCE_SYNC_AOF_START, similar to
REDISMODULE_SUBEVENT_PERSISTENCE_AOF_START which is async.

Note: This PR initially also disabled BGREWRITEAOF when appendonly config is no, but that part was later removed to be discussed separately.

@chenyang8094
Copy link
Collaborator Author

chenyang8094 commented Jan 11, 2022

Add some of my comments at the top:

I'm not sure if disabling BGREWRITEAOF while AOF is off will cause problems for existing users, it's a big breaking change after all. I hope that people who have this usage scenarios can submit them so that we can immediately discard this PR

This will also affects the way some tests , For example if you want to test whether aofrewrite of a data type is correct, Before this you can do:

r bgrewriteaof
waitForBgrewriteaof r
r debug loadaof

Regardless of whether redis-server is started with appendonly yes configuration

But now you should force set appendonly yes in advance , or the test will faild.

So I changed a lot of existing tests that depended on this way.

Ironically, if I executeconfig set appendonly yesin advance, it will trigger an AOFRW itself, but to make the test more straightforward, I still have to execute the BGREWRITEAOF command separately

@oranagra oranagra changed the title Disable AOFRW when AOF is off Always create base AOF file in preamble mode. Disable AOFRW when AOF is off. Jan 11, 2022
@oranagra oranagra added 7.0-must-have breaking-change This change can potentially break existing application release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus labels Jan 11, 2022
@oranagra oranagra linked an issue Jan 11, 2022 that may be closed by this pull request
Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

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

haven't reviewed the tests yet.

src/aof.c Outdated Show resolved Hide resolved
src/aof.c Outdated Show resolved Hide resolved
src/aof.c Outdated Show resolved Hide resolved
@chenyang8094 chenyang8094 requested a review from oranagra January 12, 2022 02:31
src/aof.c Outdated Show resolved Hide resolved
src/aof.c Show resolved Hide resolved
tests/integration/aof-multi-part.tcl Outdated Show resolved Hide resolved
tests/integration/aof-multi-part.tcl Outdated Show resolved Hide resolved
tests/integration/aof-multi-part.tcl Outdated Show resolved Hide resolved
tests/integration/aof-multi-part.tcl Outdated Show resolved Hide resolved
tests/integration/dismiss-mem.tcl Outdated Show resolved Hide resolved
tests/unit/aofrw.tcl Outdated Show resolved Hide resolved
tests/unit/moduleapi/datatype2.tcl Outdated Show resolved Hide resolved
tests/unit/other.tcl Outdated Show resolved Hide resolved
tests/unit/other.tcl Outdated Show resolved Hide resolved
@chenyang8094 chenyang8094 changed the title Always create base AOF file in preamble mode. Disable AOFRW when AOF is off. Always create base AOF file when redis start from empty. Jan 12, 2022
@chenyang8094 chenyang8094 requested a review from oranagra January 12, 2022 14:20
Copy link
Member

@oranagra oranagra left a comment

Choose a reason for hiding this comment

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

i agree it's a good idea to remove the change about blocking BGREWRITEAOF from this PR.
please mark all the comments that where related to the reverted changes as resolved, and please post your comments about the use case or existing users for that in #9794

src/aof.c Outdated Show resolved Hide resolved
src/module.c Outdated Show resolved Hide resolved
src/rdb.c Outdated Show resolved Hide resolved
chenyang8094 and others added 4 commits January 13, 2022 07:45
or if we go back to the previous version, it could be two groups of nested if-else.
i.e. the problem with the first version was just the wrong placement of the `}`, but the later version with indentation and without `{}` was confusing.
Copy link
Collaborator

@soloestoy soloestoy left a comment

Choose a reason for hiding this comment

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

I'm happy that we keep BGREWRITEAOF, avoid a breaking change, it's useful in many scenarios.

@oranagra
Copy link
Member

@soloestoy we removed it from this PR to be discussed separately, but i'm not sure we reached a decision yet.
can you please write some post in the other issue about who's using it (what that change would break), and which are the many scenarios it's useful for.

@oranagra oranagra merged commit e9bff79 into redis:unstable Jan 13, 2022
@ShooterIT
Copy link
Collaborator

Hi @oranagra @soloestoy IIRC, some users actually use this feature 😂, AOF is easy to inject to another redis server when running, such as by nc , but RDB is needed to parse, enabling AOF may cause performance loss, so they execute BGREWRITEAOF when needed.

@oranagra
Copy link
Member

@ShooterIT thank. can you move that message to #9794 (we took that content out of this PR)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change This change can potentially break existing application release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants