Always create base AOF file when redis start from empty. by chenyang8094 · Pull Request #10102 · redis/redis

chenyang8094 · 2022-01-11T14:11:34Z

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 · 2022-01-11T14:17:13Z

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

haven't reviewed the tests yet.

src/aof.c

tests/integration/aof-multi-part.tcl

tests/integration/dismiss-mem.tcl

tests/unit/aofrw.tcl

tests/unit/moduleapi/datatype2.tcl

tests/unit/other.tcl

oranagra

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

src/module.c

src/rdb.c

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.

soloestoy

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

oranagra · 2022-01-13T06:32:47Z

@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.

ShooterIT · 2022-01-13T13:04:42Z

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 · 2022-01-13T13:16:37Z

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

Disable AOFRW when AOF is off

b9fc0fa

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 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 linked an issue Jan 11, 2022 that may be closed by this pull request

Additional AOFRW deprecation cleanups #9794

Open

oranagra reviewed Jan 11, 2022

View reviewed changes

src/aof.c Outdated Show resolved Hide resolved

src/aof.c Outdated Show resolved Hide resolved

src/aof.c Outdated Show resolved Hide resolved

Use foreground rdbSave to create BASE

c4c0dc0

chenyang8094 requested a review from oranagra January 12, 2022 02:31

Update test

f4babec

chenyang8094 closed this Jan 12, 2022

chenyang8094 reopened this Jan 12, 2022

Use rewriteAppendOnlyFile to create base file

5c1657c

chenyang8094 commented Jan 12, 2022

View reviewed changes

src/aof.c Outdated Show resolved Hide resolved

Always create base file

5d4cd95

oranagra reviewed Jan 12, 2022

View reviewed changes

chenyang8094 added 3 commits January 12, 2022 16:48

Update test

89d0907

Add REDISMODULE_SUBEVENT_PERSISTENCE_SYNC_AOF_START event

94b839c

Roolback the logic of disable BGREWRITEAOF

5809a2a

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 requested a review from oranagra January 12, 2022 14:20

oranagra reviewed Jan 12, 2022

View reviewed changes

src/aof.c Outdated Show resolved Hide resolved

src/module.c Outdated Show resolved Hide resolved

Update comments

e0b5c25

chenyang8094 requested a review from oranagra January 12, 2022 15:31

oranagra removed a link to an issue Jan 12, 2022

Additional AOFRW deprecation cleanups #9794

Open

Rollback rewriteAppendOnlyFile

e5c4cbf

oranagra approved these changes Jan 12, 2022

View reviewed changes

src/rdb.c Outdated Show resolved Hide resolved

yossigo approved these changes Jan 12, 2022

View reviewed changes

chenyang8094 and others added 4 commits January 13, 2022 07:45

Use single if-else

d0c53a7

Add comments

fdd0536

this is what i meant by single if-else chain..

70a9475

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.

GH editor messed it up

f7854ca

soloestoy approved these changes Jan 13, 2022

View reviewed changes

oranagra merged commit e9bff79 into redis:unstable Jan 13, 2022

oranagra mentioned this pull request Jan 14, 2022

Additional AOFRW deprecation cleanups #9794

Open

Conversation

chenyang8094 commented Jan 11, 2022 • edited by oranagra Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chenyang8094 commented Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

soloestoy left a comment

Choose a reason for hiding this comment

Uh oh!

oranagra commented Jan 13, 2022

Uh oh!

ShooterIT commented Jan 13, 2022

Uh oh!

oranagra commented Jan 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

chenyang8094 commented Jan 11, 2022 •

edited by oranagra

Loading

chenyang8094 commented Jan 11, 2022 •

edited

Loading