Unverified Commit 65e4bce0 authored by YaacovHazan's avatar YaacovHazan Committed by GitHub
Browse files

fix return value of loadAppendOnlyFiles (#10295)



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.
Co-authored-by: default avatarchenyang8094 <chenyang8094@users.noreply.github.com>
Co-authored-by: default avatarOran Agra <oran@redislabs.com>
parent fad0b0d2
...@@ -42,8 +42,8 @@ ...@@ -42,8 +42,8 @@
#include <sys/param.h> #include <sys/param.h>
void freeClientArgv(client *c); void freeClientArgv(client *c);
off_t getAppendOnlyFileSize(sds filename); off_t getAppendOnlyFileSize(sds filename, int *status);
off_t getBaseAndIncrAppendOnlyFilesSize(aofManifest *am); off_t getBaseAndIncrAppendOnlyFilesSize(aofManifest *am, int *status);
int getBaseAndIncrAppendOnlyFilesNum(aofManifest *am); int getBaseAndIncrAppendOnlyFilesNum(aofManifest *am);
int aofFileExist(char *filename); int aofFileExist(char *filename);
int rewriteAppendOnlyFile(char *filename); int rewriteAppendOnlyFile(char *filename);
...@@ -728,7 +728,7 @@ void aofOpenIfNeededOnServerStart(void) { ...@@ -728,7 +728,7 @@ void aofOpenIfNeededOnServerStart(void) {
exit(1); exit(1);
} }
server.aof_last_incr_size = getAppendOnlyFileSize(aof_name); server.aof_last_incr_size = getAppendOnlyFileSize(aof_name, NULL);
} }
int aofFileExist(char *filename) { int aofFileExist(char *filename) {
...@@ -1556,7 +1556,7 @@ cleanup: ...@@ -1556,7 +1556,7 @@ cleanup:
/* Load the AOF files according the aofManifest pointed by am. */ /* Load the AOF files according the aofManifest pointed by am. */
int loadAppendOnlyFiles(aofManifest *am) { int loadAppendOnlyFiles(aofManifest *am) {
serverAssert(am != NULL); serverAssert(am != NULL);
int ret = C_OK; int status, ret = C_OK;
long long start; long long start;
off_t total_size = 0; off_t total_size = 0;
sds aof_name; sds aof_name;
...@@ -1590,7 +1590,16 @@ int loadAppendOnlyFiles(aofManifest *am) { ...@@ -1590,7 +1590,16 @@ int loadAppendOnlyFiles(aofManifest *am) {
/* Here we calculate the total size of all BASE and INCR files in /* Here we calculate the total size of all BASE and INCR files in
* advance, it will be set to `server.loading_total_bytes`. */ * advance, it will be set to `server.loading_total_bytes`. */
total_size = getBaseAndIncrAppendOnlyFilesSize(am); total_size = getBaseAndIncrAppendOnlyFilesSize(am, &status);
if (status != AOF_OK) {
/* If an AOF exists in the manifest but not on the disk, we consider this to be a fatal error. */
if (status == AOF_NOT_EXIST) status = AOF_FAILED;
return status;
} else if (total_size == 0) {
return AOF_EMPTY;
}
startLoading(total_size, RDBFLAGS_AOF_PREAMBLE, 0); startLoading(total_size, RDBFLAGS_AOF_PREAMBLE, 0);
/* Load BASE AOF if needed. */ /* Load BASE AOF if needed. */
...@@ -1606,9 +1615,8 @@ int loadAppendOnlyFiles(aofManifest *am) { ...@@ -1606,9 +1615,8 @@ int loadAppendOnlyFiles(aofManifest *am) {
aof_name, (float)(ustime()-start)/1000000); aof_name, (float)(ustime()-start)/1000000);
} }
/* If an AOF exists in the manifest but not on the disk, Or the truncated /* If the truncated file is not the last file, we consider this to be a fatal error. */
* file is not the last file, we consider this to be a fatal error. */ if (ret == AOF_TRUNCATED && !last_file) {
if (ret == AOF_NOT_EXIST || (ret == AOF_TRUNCATED && !last_file)) {
ret = AOF_FAILED; ret = AOF_FAILED;
} }
...@@ -1636,7 +1644,11 @@ int loadAppendOnlyFiles(aofManifest *am) { ...@@ -1636,7 +1644,11 @@ int loadAppendOnlyFiles(aofManifest *am) {
aof_name, (float)(ustime()-start)/1000000); aof_name, (float)(ustime()-start)/1000000);
} }
if (ret == AOF_NOT_EXIST || (ret == AOF_TRUNCATED && !last_file)) { /* We know that (at least) one of the AOF files has data (total_size > 0),
* so empty incr AOF file doesn't count as a AOF_EMPTY result */
if (ret == AOF_EMPTY) ret = AOF_OK;
if (ret == AOF_TRUNCATED && !last_file) {
ret = AOF_FAILED; ret = AOF_FAILED;
} }
...@@ -1651,7 +1663,7 @@ int loadAppendOnlyFiles(aofManifest *am) { ...@@ -1651,7 +1663,7 @@ int loadAppendOnlyFiles(aofManifest *am) {
server.aof_fsync_offset = server.aof_current_size; server.aof_fsync_offset = server.aof_current_size;
cleanup: cleanup:
stopLoading(ret == AOF_OK); stopLoading(ret == AOF_OK || ret == AOF_TRUNCATED);
return ret; return ret;
} }
...@@ -2404,7 +2416,10 @@ void aofRemoveTempFile(pid_t childpid) { ...@@ -2404,7 +2416,10 @@ void aofRemoveTempFile(pid_t childpid) {
bg_unlink(tmpfile); bg_unlink(tmpfile);
} }
off_t getAppendOnlyFileSize(sds filename) { /* Get size of an AOF file.
* The status argument is an optional output argument to be filled with
* one of the AOF_ status values. */
off_t getAppendOnlyFileSize(sds filename, int *status) {
struct redis_stat sb; struct redis_stat sb;
off_t size; off_t size;
mstime_t latency; mstime_t latency;
...@@ -2412,10 +2427,12 @@ off_t getAppendOnlyFileSize(sds filename) { ...@@ -2412,10 +2427,12 @@ off_t getAppendOnlyFileSize(sds filename) {
sds aof_filepath = makePath(server.aof_dirname, filename); sds aof_filepath = makePath(server.aof_dirname, filename);
latencyStartMonitor(latency); latencyStartMonitor(latency);
if (redis_stat(aof_filepath, &sb) == -1) { if (redis_stat(aof_filepath, &sb) == -1) {
if (status) *status = errno == ENOENT ? AOF_NOT_EXIST : AOF_OPEN_ERR;
serverLog(LL_WARNING, "Unable to obtain the AOF file %s length. stat: %s", serverLog(LL_WARNING, "Unable to obtain the AOF file %s length. stat: %s",
filename, strerror(errno)); filename, strerror(errno));
size = 0; size = 0;
} else { } else {
if (status) *status = AOF_OK;
size = sb.st_size; size = sb.st_size;
} }
latencyEndMonitor(latency); latencyEndMonitor(latency);
...@@ -2424,22 +2441,27 @@ off_t getAppendOnlyFileSize(sds filename) { ...@@ -2424,22 +2441,27 @@ off_t getAppendOnlyFileSize(sds filename) {
return size; return size;
} }
off_t getBaseAndIncrAppendOnlyFilesSize(aofManifest *am) { /* Get size of all AOF files referred by the manifest (excluding history).
* The status argument is an output argument to be filled with
* one of the AOF_ status values. */
off_t getBaseAndIncrAppendOnlyFilesSize(aofManifest *am, int *status) {
off_t size = 0; off_t size = 0;
listNode *ln; listNode *ln;
listIter li; listIter li;
if (am->base_aof_info) { if (am->base_aof_info) {
serverAssert(am->base_aof_info->file_type == AOF_FILE_TYPE_BASE); serverAssert(am->base_aof_info->file_type == AOF_FILE_TYPE_BASE);
size += getAppendOnlyFileSize(am->base_aof_info->file_name);
size += getAppendOnlyFileSize(am->base_aof_info->file_name, status);
if (*status != AOF_OK) return 0;
} }
listRewind(am->incr_aof_list, &li); listRewind(am->incr_aof_list, &li);
while ((ln = listNext(&li)) != NULL) { while ((ln = listNext(&li)) != NULL) {
aofInfo *ai = (aofInfo*)ln->value; aofInfo *ai = (aofInfo*)ln->value;
serverAssert(ai->file_type == AOF_FILE_TYPE_INCR); serverAssert(ai->file_type == AOF_FILE_TYPE_INCR);
size += getAppendOnlyFileSize(ai->file_name); size += getAppendOnlyFileSize(ai->file_name, status);
if (*status != AOF_OK) return 0;
} }
return size; return size;
...@@ -2513,7 +2535,7 @@ void backgroundRewriteDoneHandler(int exitcode, int bysignal) { ...@@ -2513,7 +2535,7 @@ void backgroundRewriteDoneHandler(int exitcode, int bysignal) {
if (server.aof_fd != -1) { if (server.aof_fd != -1) {
/* AOF enabled. */ /* AOF enabled. */
server.aof_selected_db = -1; /* Make sure SELECT is re-issued */ server.aof_selected_db = -1; /* Make sure SELECT is re-issued */
server.aof_current_size = getAppendOnlyFileSize(new_base_filename) + server.aof_last_incr_size; server.aof_current_size = getAppendOnlyFileSize(new_base_filename, NULL) + server.aof_last_incr_size;
server.aof_rewrite_base_size = server.aof_current_size; server.aof_rewrite_base_size = server.aof_current_size;
server.aof_fsync_offset = server.aof_current_size; server.aof_fsync_offset = server.aof_current_size;
server.aof_last_fsync = server.unixtime; server.aof_last_fsync = server.unixtime;
......
...@@ -45,7 +45,7 @@ tags {"external:skip"} { ...@@ -45,7 +45,7 @@ tags {"external:skip"} {
fail "AOF loading didn't fail" fail "AOF loading didn't fail"
} }
assert_equal 1 [count_message_lines $server_path/stdout "appendonly.aof.1.incr.aof doesn't exist"] assert_equal 1 [count_message_lines $server_path/stdout "appendonly.aof.1.incr.aof .*No such file or directory"]
} }
clean_aof_persistence $aof_dirpath clean_aof_persistence $aof_dirpath
...@@ -584,7 +584,7 @@ tags {"external:skip"} { ...@@ -584,7 +584,7 @@ tags {"external:skip"} {
fail "AOF loading didn't fail" fail "AOF loading didn't fail"
} }
assert_equal 1 [count_message_lines $server_path/stdout "appendonly.aof doesn't exist"] assert_equal 1 [count_message_lines $server_path/stdout "appendonly.aof .*No such file or directory"]
} }
clean_aof_persistence $aof_dirpath clean_aof_persistence $aof_dirpath
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment