cleanup around loadAppendOnlyFile by YaacovHazan · Pull Request #9012 · redis/redis

YaacovHazan · 2021-05-31T12:14:27Z

Today when we load the AOF on startup, the loadAppendOnlyFile checks if
the file is openning for reading.
This check is redundent (dead code) as we open the AOF file for writing at initServer,
and the file will always be existing for the loadAppendOnlyFile.

In this commit:

remove all the exit(1) from loadAppendOnlyFile, as it is the caller
responsibility to decide what to do in case of failure.
move the opening of the AOF file for writing, to be after we loading it.
avoid return -ERR in DEBUG LOADAOF, when the AOF is existing but empty

oranagra

can you please verify that we have some tests to cover these error cases and try to run them with valgrind

src/server.h

src/aof.c

src/debug.c

Today when we load the AOF on startup, the loadAppendOnlyFile checks if the file is openning for reading. This check is redundent (dead code) as we open the AOF file at initServer, and the file will always be existing for the loadAppendOnlyFile. In this commit: - remove all the exit(1) from loadAppendOnlyFile, as it is the caller responsibility to decide what to do in case of failure. - move the opening of the AOF file for writing, to be after we loading it. - avoid return -ERR in DEBUG LOADAOF, when the AOF is existing but empty

tests/integration/aof.tcl

oranagra · 2021-06-01T11:49:23Z

@yossigo i'd like you to approve this change too.

yossigo

LGTM, but I want to point out this is not just cleanup but will have some functional impact in rare (unexpected?) conditions:

Before this commit, if the file was replaced in the right moment we could end up creating AOF file A and reading from AOF file B. Later on, we'd keep writing to AOF file A (which could even have been unlinked at this point).
Before this commit, a non-writable AOF would be caught before reading it. Now we'll hit the error and bail only after reading.

src/aof.c

src/server.c

soloestoy · 2021-06-08T09:29:11Z

should we also change rdb?

oranagra · 2021-06-08T09:32:59Z

should we also change rdb?

@soloestoy what do you mean? change rdb.c not to use exit(1) yes, at some point (we wanna create librdb), but not now.
regarding the order of write and read, there's no issue there, maybe i'm missing your point.

soloestoy · 2021-06-08T09:43:54Z

@oranagra I didn't make it clear, what I mean is:

if we use aof-use-rdb-preamble, function loadAppendOnlyFile() will call rdbLoadRio() and may use exit(1).
this PR seems aim to let the caller(now is only loadDataFromDisk()) to decide if it should exit(1) or not, so I think load AOF and RDB should have same behavior.
as you said, we wanna create librdb.

- change the top comment of loadAppendOnlyFile() to reflect the new return values. - handle differently, the case of AOF file does not exist, and AOF exist but doesn't open for reading

YaacovHazan · 2021-06-08T12:12:02Z

@soloestoy you are right. This PR is more about the order of opening the files, and the error cleanup is the bonus.
Another deeper PR is needed to sort it out in rdb.c

Today when we load the AOF on startup, the loadAppendOnlyFile checks if the file is openning for reading. This check is redundent (dead code) as we open the AOF file for writing at initServer, and the file will always be existing for the loadAppendOnlyFile. In this commit: - remove all the exit(1) from loadAppendOnlyFile, as it is the caller responsibility to decide what to do in case of failure. - move the opening of the AOF file for writing, to be after we loading it. - avoid return -ERR in DEBUG LOADAOF, when the AOF is existing but empty

this was a regression from #9012 (not released yet)

enjoy-binbin · 2023-02-08T03:25:52Z

src/debug.c

+        if (ret != AOF_OK && ret != AOF_EMPTY)
+            exit(1);


Why did we choose exit here. A DEBUG command can make the server exit, is a bit odd.

I encountered this issue:
start a server with AOF off
debug loadaof
the server exit

the reason is that, in loadAppendOnlyFiles, the ret is AOF_NOT_EXIST, so it do a exit(1)

i think we should also ignore AOF_NOT_EXIST in this case?
or actually we need to return an error, print the log, instead of exit

this command is only meant to be used by the test suite, and only by tests that generated an AOF file first.
i don't think it should ignore the AOF_NOT_EXIST, but i'm willing to trade this exit with an error reply (considering that the caller is likely to catch this error and die)

Return an error when loadAppendOnlyFiles fails instead of exiting. DEBUF LOADAOF command is only meant to be used by the test suite, and only by tests that generated an AOF file first. So this change is ok (considering that the caller is likely to catch this error and die). This actually revert part of the code in redis#9012, and now DEBUG LOADAOF behaves the same as DEBUG RELOAD (returns an error when the load fails). Plus remove a `after 2000` in a test, which can save times.

Return an error when loadAppendOnlyFiles fails instead of exiting. DEBUF LOADAOF command is only meant to be used by the test suite, and only by tests that generated an AOF file first. So this change is ok (considering that the caller is likely to catch this error and die). This actually revert part of the code in #9012, and now DEBUG LOADAOF behaves the same as DEBUG RELOAD (returns an error when the load fails). Plus remove a `after 2000` in a test, which can save times (looks like copy paste error).

…1790) Return an error when loadAppendOnlyFiles fails instead of exiting. DEBUF LOADAOF command is only meant to be used by the test suite, and only by tests that generated an AOF file first. So this change is ok (considering that the caller is likely to catch this error and die). This actually revert part of the code in redis#9012, and now DEBUG LOADAOF behaves the same as DEBUG RELOAD (returns an error when the load fails). Plus remove a `after 2000` in a test, which can save times (looks like copy paste error).

YaacovHazan requested a review from oranagra May 31, 2021 12:14

oranagra reviewed May 31, 2021

View reviewed changes

src/server.h Outdated Show resolved Hide resolved

src/aof.c Outdated Show resolved Hide resolved

src/debug.c Outdated Show resolved Hide resolved

YaacovHazan force-pushed the clean_load_append_only branch from 5f04aa6 to 0f06cba Compare June 1, 2021 10:42

oranagra reviewed Jun 1, 2021

View reviewed changes

tests/integration/aof.tcl Show resolved Hide resolved

oranagra previously approved these changes Jun 1, 2021

View reviewed changes

oranagra added the state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten label Jun 1, 2021

yossigo reviewed Jun 7, 2021

View reviewed changes

src/aof.c Show resolved Hide resolved

src/server.c Outdated Show resolved Hide resolved

soloestoy reviewed Jun 8, 2021

View reviewed changes

src/server.c Outdated Show resolved Hide resolved

cleanup around loadAppendOnlyFile (2)

3cd98a0

- change the top comment of loadAppendOnlyFile() to reflect the new return values. - handle differently, the case of AOF file does not exist, and AOF exist but doesn't open for reading

YaacovHazan dismissed oranagra’s stale review via 3cd98a0 June 8, 2021 12:02

oranagra approved these changes Jun 9, 2021

View reviewed changes

oranagra merged commit 1677efb into redis:unstable Jun 14, 2021

oranagra mentioned this pull request Sep 16, 2021

Fix obtain the AOF file length error when load AOF #9510

Merged

oranagra pushed a commit that referenced this pull request Sep 26, 2021

Fix obtain the AOF file length error when load AOF (#9510)

7c1f9ef

this was a regression from #9012 (not released yet)

chenyang8094 mentioned this pull request Apr 21, 2022

Fix bug when AOF enabled after startup. put the new incr file in the manifest only when AOFRW is done. #10616

Merged

enjoy-binbin mentioned this pull request Jul 26, 2022

Add error log message when failing to open RDB file for reading #11036

Merged

enjoy-binbin reviewed Feb 8, 2023

View reviewed changes

enjoy-binbin mentioned this pull request Feb 8, 2023

When DEBUG LOADAOF fails, return an error instead of exiting #11790

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cleanup around loadAppendOnlyFile#9012

cleanup around loadAppendOnlyFile#9012
oranagra merged 2 commits intoredis:unstablefrom
YaacovHazan:clean_load_append_only

YaacovHazan commented May 31, 2021 •

edited by oranagra

Loading

Uh oh!

oranagra left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oranagra commented Jun 1, 2021

Uh oh!

yossigo left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

soloestoy commented Jun 8, 2021

Uh oh!

oranagra commented Jun 8, 2021

Uh oh!

soloestoy commented Jun 8, 2021

Uh oh!

YaacovHazan commented Jun 8, 2021

Uh oh!

enjoy-binbin Feb 8, 2023

Uh oh!

oranagra Feb 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

YaacovHazan commented May 31, 2021 • edited by oranagra 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!

oranagra commented Jun 1, 2021

Uh oh!

yossigo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

soloestoy commented Jun 8, 2021

Uh oh!

oranagra commented Jun 8, 2021

Uh oh!

soloestoy commented Jun 8, 2021

Uh oh!

YaacovHazan commented Jun 8, 2021

Uh oh!

enjoy-binbin Feb 8, 2023

Choose a reason for hiding this comment

Uh oh!

oranagra Feb 8, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

YaacovHazan commented May 31, 2021 •

edited by oranagra

Loading