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.

Sign up for GitHub

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

Jump to bottom

Rename redis in aof logs and proc title redis-aof-rewrite to valkey-aof-rewrite. #393

Merged
merged 7 commits into from May 1, 2024
Merged

Rename redis in aof logs and proc title redis-aof-rewrite to valkey-aof-rewrite. #393

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
79ca612
Rename redis in aof logs
Shivshankar-Reddy Apr 26, 2024
54ac594
correct log statement
Shivshankar-Reddy Apr 29, 2024
64095cf
update all logs
Shivshankar-Reddy Apr 29, 2024
c27cb4c
update proc name based on symlink
Shivshankar-Reddy Apr 30, 2024
9af3bb2
correct symlink name
Shivshankar-Reddy Apr 30, 2024
ed99e67
correct spellcheck
Shivshankar-Reddy Apr 30, 2024
00d7cb5
Remove the bool and compare with argv
Shivshankar-Reddy May 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 7 additions & 7 deletions src/aof.c
View file
Open in desktop
Expand Up @@ -996,7 +996,7 @@ int startAppendOnly(void) {

if (rewriteAppendOnlyFileBackground() == C_ERR) {
server.aof_state = AOF_OFF;
serverLog(LL_WARNING,"Redis needs to enable the AOF but can't trigger a background AOF rewrite operation. Check the above logs for more info about the error.");
serverLog(LL_WARNING,"Server needs to enable the AOF but can't trigger a background AOF rewrite operation. Check the above logs for more info about the error.");
Copy link
Contributor

Choose a reason for hiding this comment

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

This is part of #207 so I will tag @0del because he is assigned to it. @0del are you still working on the remaining logging?

In #207 we discussed this style:

serverLog(LL_WARNING,"%s needs to enable the AOF but can't trigger a background AOF rewrite operation. Check the above logs for more info about the error.",
          SERVER_TITLE);

... because then it would be easy patch the SERVER_TITLE macro to be "Redis" to get the old logging. But then we discussed it a bit more and decided that most of the logging isn't a problem about redis compatibility, so I think we don't need this complexity.

I'm OK with "Server", but probably better grammar is "The server" in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review, Changed the string "Server" to "The server" and updated the diff.

return C_ERR;
}
}
Expand Down Expand Up @@ -1121,7 +1121,7 @@ void flushAppendOnlyFile(int force) {
/* Otherwise fall through, and go write since we can't wait
* over two seconds. */
server.aof_delayed_fsync++;
serverLog(LL_NOTICE,"Asynchronous AOF fsync is taking too long (disk is busy?). Writing the AOF buffer without waiting for fsync to complete, this may slow down Redis.");
serverLog(LL_NOTICE,"Asynchronous AOF fsync is taking too long (disk is busy?). Writing the AOF buffer without waiting for fsync to complete, this may slow down the server.");
}
}
/* We want to perform a single write. This should be guaranteed atomic
Expand Down Expand Up @@ -1183,7 +1183,7 @@ void flushAppendOnlyFile(int force) {
if (ftruncate(server.aof_fd, server.aof_last_incr_size) == -1) {
if (can_log) {
serverLog(LL_WARNING, "Could not remove short write "
"from the append-only file. Redis may refuse "
"from the append-only file. Server may refuse "
"to load the AOF the next time it starts. "
"ftruncate: %s", strerror(errno));
}
Expand Down Expand Up @@ -1224,7 +1224,7 @@ void flushAppendOnlyFile(int force) {
* OK state and log the event. */
if (server.aof_last_write_status == C_ERR) {
serverLog(LL_NOTICE,
"AOF write error looks solved, Redis can write again.");
"AOF write error looks solved, Server can write again.");
server.aof_last_write_status = C_OK;
}
}
Expand Down Expand Up @@ -1634,14 +1634,14 @@ int loadSingleAppendOnlyFile(char *filename) {
}
}
serverLog(LL_WARNING, "Unexpected end of file reading the append only file %s. You can: "
"1) Make a backup of your AOF file, then use ./redis-check-aof --fix <filename.manifest>. "
"1) Make a backup of your AOF file, then use ./valkey-check-aof --fix <filename.manifest>. "
"2) Alternatively you can set the 'aof-load-truncated' configuration option to yes and restart the server.", filename);
ret = AOF_FAILED;
goto cleanup;

fmterr: /* Format error. */
serverLog(LL_WARNING, "Bad file format reading the append only file %s: "
"make a backup of your AOF file, then use ./redis-check-aof --fix <filename.manifest>", filename);
"make a backup of your AOF file, then use ./valkey-check-aof --fix <filename.manifest>", filename);
ret = AOF_FAILED;
/* fall through to cleanup. */

Expand Down Expand Up @@ -2471,7 +2471,7 @@ int rewriteAppendOnlyFileBackground(void) {
char tmpfile[256];

/* Child */
serverSetProcTitle("redis-aof-rewrite");
serverSetProcTitle("valkey-aof-rewrite");
Copy link
Contributor

Choose a reason for hiding this comment

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

@PingXie You're right that proc title is not logging.

For the most reasonable result we could check if Valkey was started using a redis-server symlink, similar to how we check if we was started as valkey-check-aof and other names, here:

valkey/src/server.c

Line 7019 in a5a1377

if (strstr(exec_name,"valkey-check-rdb") != NULL)

Maybe it's too complex and not important. I'm not sure.

Copy link
Member

Choose a reason for hiding this comment

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

check if Valkey was started using a redis-server symlink

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have tried to add a flag similar to server.sentinel_mode, I am still not sure whether it is appropriate and best way.

serverSetCpuAffinity(server.aof_rewrite_cpulist);
snprintf(tmpfile,256,"temp-rewriteaof-bg-%d.aof", (int) getpid());
if (rewriteAppendOnlyFile(tmpfile) == C_OK) {
Expand Down
2 changes: 1 addition & 1 deletion src/cluster_legacy.c
View file
Open in desktop
Expand Up @@ -808,7 +808,7 @@ int clusterLockConfig(char *filename) {
* we need save `fd` to `cluster_config_file_lock_fd`, so that in serverFork(),
* it will be closed in the child process.
* If it is not closed, when the main process is killed -9, but the child process
* (redis-aof-rewrite) is still alive, the fd(lock) will still be held by the
* (valkey-aof-rewrite) is still alive, the fd(lock) will still be held by the
* child process, and the main process will fail to get lock, means fail to start. */
server.cluster_config_file_lock_fd = fd;
#else
Expand Down