fix return value of loadAppendOnlyFiles by YaacovHazan · Pull Request #10295 · redis/redis

YaacovHazan · 2022-02-13T21:49:07Z

Make sure the status return from loading multiple AOF files reflects the overall
result, not just the one of the last file.

When one of the AOF files succeeded to load, but the last AOF file
was empty, the loadAppendOnlyFiles will return AOF_EMPTY.
This commit changes this behavior, and return AOF_OK in that case.

This can happen for example, when loading old AOF file, and no more commands processed,
the manifest file will include base AOF file with data, and empty incr AOF file.

When one of the AOF files succeeded to load, but the last AOF file was empty, the loadAppendOnlyFiles will return AOF_EMPTY. This commit changes this behavior, and return AOF_OK in that case. This can happen for example, when loading old AOF file, and no more commands processed, the manifest file will include base AOF file with data, and empty incr AOF file.

chenyang8094

Thanks, I found out that the return value of AOF was introduced in #9012 (which you submitted). In this PR, AOF_EMPTY is considered a correct return value (such as in debug loadaof).

But in multi part aof, both base and incr AOF may be empty. The current implementation is to directly ignore the empty return value during the AOF loading process, and finally return it to the upper-level caller for processing.

But I found that at the end of the loadAppendOnlyFiles function, there is a problem when we use stopLoading(ret == AOF_OK) At this time, if ret is AOF_EMPTY, we will send a wrong load event to the module, which is not as expected. So I think this modification of yours makes sense. Or change to stopLoading(ret == AOF_OK || ret == AOF_EMPTY) to use less code to get the same effect.

In addition, according to your modification, the upper-level caller will no longer perceive AOF_EMPTY, so can we remove judgment on AOF_EMPTY in debug.c?

oranagra · 2022-02-14T06:57:11Z

I think it's a good idea to keep supporting the EMPTY return value, some callers may want to treat that as a failure (e.g debug.c, or if we some day have an explicit AOFLOAD command).
but we should return empty only if the entire AOF loading was empty (i.e. we didn't load anything).
if the base wasn't empty, and just the INCR was empty, we wanna return OK.

as for stopLoading. ideally, we could have left that for the caller to decide what's a failure, or maybe pass in some flag telling loadAppendOnlyFiles which values are a failure, but that's too complicated.
i suppose we can be liberal here and only treat fatal errors as stopLoading failures (i.e. AOF_FAILED and maybe AOF_OPEN_ERR).
IIRC the main user of this indication is modules listening to the event so that they can maybe revert / release something that was allocated during loading, but that's mainly important in replica rdb loading since the replica can keep running, and doesn't seem important in AOF loading (in which redis will exit on failure).

oranagra · 2022-02-14T06:59:49Z

src/aof.c

+    if (aof_loaded && ret == AOF_EMPTY) ret = AOF_OK;
+
 cleanup:
    stopLoading(ret == AOF_OK);


i think we need to treat TRUNCATED and maybe EMPTY as success..

Agree.

if no base and no incr AOF，we just return AOF_NOT_EXIST

or if total_size == 0 ， we just return AOF_EMPTY

if any of the middle AOF is empty ,we convert the ret to AOF_OK

if the AOF was truncated (only the last file) , we also treat it as AOF_OK

@oranagra right ?

yes, that's the logic i aim for.
please check the code to make sure we didn't miss anything.

@YaacovHazan Maybe you need to update your PR to satisfy the above logic. thanks.

@chenyang8094 already added a new commit... pls review

YaacovHazan · 2022-02-14T07:11:38Z

@chenyang8094 thanks for the review.
We still can get AOF_EMPTY in debug.c (probably not common use case), if aof-use-rdb-preamble is disabled and no data was saved to AOF, So I think we should leave it as is.

treat TRUNCATED and EMPTY as success

no need for extra aof_loaded vairable

YaacovHazan · 2022-02-14T19:21:33Z

@chenyang8094 I pushed a new commit, please review.
note that we cannot do the check of the total_size before we actually try to load the AOF files, because missing files counted as a zero size.

fix compliation

chenyang8094 · 2022-02-15T01:36:02Z

@chenyang8094 I pushed a new commit, please review. note that we cannot do the check of the total_size before we actually try to load the AOF files, because missing files counted as a zero size.

You are right. Maybe I should have made getBaseAndIncrAppendOnlyFilesSize return -1 when there is an error (such as some AOF files do not exist), so that I can return AOF_FAILED directly in loadAppendOnlyFiles. Because if I can judge in advance that all AOFs is empty, I can avoid executing a meaningless startloading and stoploading.

make getBaseAndIncrAppendOnlyFilesSize return extra information if we succeeded or not to get the AOF files size

YaacovHazan · 2022-02-15T07:40:27Z

@chenyang8094 makes sense... I changed the getBaseAndIncrAppendOnlyFilesSize & getAppendOnlyFileSize to return status.

fix the usage of status pointer

some more fixes

chenyang8094 · 2022-02-17T12:18:49Z

@chenyang8094 makes sense... I changed the getBaseAndIncrAppendOnlyFilesSize & getAppendOnlyFileSize to return status.

@YaacovHazan Please solve the test error, thanks.

chenyang8094

LGTM

src/aof.c

chenyang8094 suggested changes Feb 14, 2022

View reviewed changes

oranagra reviewed Feb 14, 2022

View reviewed changes

YaacovHazan added 2 commits February 14, 2022 09:29

fix return value of loadAppendOnlyFiles (2)

cae4528

treat TRUNCATED and EMPTY as success

fix return value of loadAppendOnlyFiles (3)

9db3787

no need for extra aof_loaded vairable

fix return value of loadAppendOnlyFiles (3)

ab1078a

fix compliation

fix return value of loadAppendOnlyFiles (5)

8dcf0d8

make getBaseAndIncrAppendOnlyFilesSize return extra information if we succeeded or not to get the AOF files size

YaacovHazan added 2 commits February 15, 2022 10:11

fix return value of loadAppendOnlyFiles (6)

e469aca

fix the usage of status pointer

fix return value of loadAppendOnlyFiles (7)

46c4a91

some more fixes

fix tests log matching, skip history files in size counting

5bfd4e9

oranagra approved these changes Feb 21, 2022

View reviewed changes

CR fix

fdb107e

chenyang8094 approved these changes Feb 21, 2022

View reviewed changes

oranagra merged commit 65e4bce into redis:unstable Feb 22, 2022

Conversation

YaacovHazan commented Feb 13, 2022 • edited by oranagra Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chenyang8094 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oranagra commented Feb 14, 2022

Uh oh!

oranagra Feb 14, 2022

Choose a reason for hiding this comment

Uh oh!

chenyang8094 Feb 14, 2022

Choose a reason for hiding this comment

Uh oh!

oranagra Feb 14, 2022

Choose a reason for hiding this comment

Uh oh!

chenyang8094 Feb 14, 2022

Choose a reason for hiding this comment

Uh oh!

YaacovHazan Feb 14, 2022

Choose a reason for hiding this comment

Uh oh!

YaacovHazan commented Feb 14, 2022

Uh oh!

YaacovHazan commented Feb 14, 2022

Uh oh!

chenyang8094 commented Feb 15, 2022

Uh oh!

YaacovHazan commented Feb 15, 2022

Uh oh!

chenyang8094 commented Feb 17, 2022

Uh oh!

chenyang8094 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

YaacovHazan commented Feb 13, 2022 •

edited by oranagra

Loading

chenyang8094 left a comment •

edited

Loading