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

Add tag for dual-channel logs #999

Merged
merged 2 commits into from
Nov 26, 2024
Merged

Add tag for dual-channel logs #999

merged 2 commits into from
Nov 26, 2024

Conversation

naglera
Copy link
Contributor

@naglera naglera commented Sep 8, 2024 •
edited by ranshid
Loading

This PR introduces a consistent tagging system for dual-channel logs. The goal is to improve log readability and filterability, making it easier for operators to manage and analyze log entries.

Resolves #986

@codecov Codecov
Copy link

codecov bot commented Sep 11, 2024

Codecov Report

Attention: Patch coverage is 54.54545% with 15 lines in your changes missing coverage. Please review.

Project coverage is 70.59%. Comparing base (4033c99) to head (e96dcc0).
Report is 1 commits behind head on unstable.

Files with missing lines Patch % Lines
src/replication.c 51.61% 15 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##           unstable     #999      +/-   ##
============================================
+ Coverage     70.55%   70.59%   +0.04%     
============================================
  Files           114      114              
  Lines         61658    61658              
============================================
+ Hits          43501    43530      +29     
+ Misses        18157    18128      -29
Files with missing lines Coverage Δ
src/networking.c 88.43% <100.00%> (ø)
src/replication.c 86.91% <51.61%> (ø)

... and 9 files with indirect coverage changes

ranshid
ranshid reviewed Nov 24, 2024
View reviewed changes
Copy link
Member

@ranshid ranshid left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Can we consider define a new DualChannel logger?

Like:

#define DualChannelServerLog(level, ...) \
    serverLog(level, "<Dual Channel> " __VA_ARGS__)

IMO it would make the logging and code cleaner

ranshid
ranshid reviewed Nov 25, 2024
View reviewed changes
Copy link
Member

@ranshid ranshid left a comment

Choose a reason for hiding this comment

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

Some places I wonder why we havn't used the macro

src/replication.c Outdated
@@ -2607,7 +2610,8 @@ static void fullSyncWithPrimary(connection *conn) {
argc++;
err = sendCommandArgv(conn, argc, args, lens);
if (err) {
serverLog(LL_WARNING, "Sending command to primary in dual channel replication handshake: %s", err);
serverLog(LL_WARNING,
Copy link
Member

Choose a reason for hiding this comment

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

why not use the macro here?

src/replication.c Outdated
goto error;
}

if (err[0] == '-') {
serverLog(LL_NOTICE,
"Server does not support sync with offset, dual channel sync approach cannot be used: %s", err);
Copy link
Member

Choose a reason for hiding this comment

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

why not replace with the macro here?

src/replication.c Outdated
cancelReplicationHandshake(1);
}
return C_ERR;
}
if (nread == 0) {
serverLog(LL_VERBOSE, "Provisional primary closed connection");
serverLog(LL_VERBOSE, "<Dual Channel> Provisional primary closed connection");
Copy link
Member

Choose a reason for hiding this comment

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

macro?

src/replication.c Outdated
@@ -3009,8 +3016,9 @@ int replicaTryPartialResynchronization(connection *conn, int read_reply) {
/* While in dual channel replication, we should use our prepared repl id and offset. */
psync_replid = server.repl_provisional_primary.replid;
snprintf(psync_offset, sizeof(psync_offset), "%lld", server.repl_provisional_primary.reploff + 1);
serverLog(LL_NOTICE, "Trying a partial resynchronization using main channel (request %s:%s).", psync_replid,
psync_offset);
serverLog(LL_NOTICE,
Copy link
Member

Choose a reason for hiding this comment

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

macro?

src/networking.c Outdated
@@ -1923,14 +1923,15 @@ int freeClientsInAsyncFreeQueue(void) {
if (!c->rdb_client_disconnect_time) {
if (c->conn) connSetReadHandler(c->conn, NULL);
c->rdb_client_disconnect_time = server.unixtime;
serverLog(LL_VERBOSE, "Postpone RDB client id=%llu (%s) free for %d seconds", (unsigned long long)c->id,
replicationGetReplicaName(c), server.wait_before_rdb_client_free);
serverLog(LL_VERBOSE, "<Dual Channel> Postpone RDB client id=%llu (%s) free for %d seconds",
Copy link
Member

Choose a reason for hiding this comment

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

why not use the macro?

src/networking.c Outdated
@@ -1682,7 +1682,7 @@ void freeClient(client *c) {
/* Log link disconnection with replica */
if (getClientType(c) == CLIENT_TYPE_REPLICA) {
serverLog(LL_NOTICE,
c->flag.repl_rdb_channel ? "Replica %s rdb channel disconnected."
c->flag.repl_rdb_channel ? "<Dual Channel> Replica %s rdb channel disconnected."
Copy link
Member

Choose a reason for hiding this comment

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

I see no harm in moving the check higher and use the macro in this case.

src/networking.c Outdated
"Replica main channel failed to establish PSYNC within the grace period (%ld seconds). "
"Freeing RDB client %llu.",
(long int)(server.unixtime - c->rdb_client_disconnect_time), (unsigned long long)c->id);
serverLog(
Copy link
Member

Choose a reason for hiding this comment

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

why not use the macro?

@naglera
Add log tag for dual-channel logs
bc9432c 
Signed-off-by: naglera <anagler123@gmail.com>
@naglera
clang fix
f2c430f 
Signed-off-by: naglera <anagler123@gmail.com>
ranshid
ranshid approved these changes Nov 26, 2024
View reviewed changes
Copy link
Member

@ranshid ranshid left a comment

Choose a reason for hiding this comment

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

I think this is a safe change and the failed test seems to be related to something else.

@ranshid
Copy link
Member

ranshid commented Nov 26, 2024

@naglera Please just add a short description in the top comment so that we do not have a link to a github comment in the git log.

@ranshid ranshid merged commit 9305b49 into valkey-io:unstable Nov 26, 2024
45 of 46 checks passed
vudiep411 pushed a commit to vudiep411/valkey that referenced this pull request Nov 26, 2024
@naglera @vudiep411
Add tag for dual-channel logs (valkey-io#999)
ccb35ee 
This PR introduces a consistent tagging system for dual-channel logs.
The goal is to improve log readability and filterability, making it
easier for operators to manage and analyze log entries.

Resolves valkey-io#986

---------

Signed-off-by: naglera <anagler123@gmail.com>
Signed-off-by: vudiep411 <vdiep@amazon.com>
vudiep411 pushed a commit to vudiep411/valkey that referenced this pull request Nov 26, 2024
@naglera @vudiep411
Add tag for dual-channel logs (valkey-io#999)
43633c6 
This PR introduces a consistent tagging system for dual-channel logs.
The goal is to improve log readability and filterability, making it
easier for operators to manage and analyze log entries.

Resolves valkey-io#986

---------

Signed-off-by: naglera <anagler123@gmail.com>
Signed-off-by: vudiep411 <vdiep@amazon.com>
vudiep411 pushed a commit to vudiep411/valkey that referenced this pull request Nov 26, 2024
@naglera @vudiep411
Add tag for dual-channel logs (valkey-io#999)
882d9dc 
This PR introduces a consistent tagging system for dual-channel logs.
The goal is to improve log readability and filterability, making it
easier for operators to manage and analyze log entries.

Resolves valkey-io#986

---------

Signed-off-by: naglera <anagler123@gmail.com>
Signed-off-by: vudiep411 <vdiep@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ranshid ranshid ranshid approved these changes

@hwware hwware hwware approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add [Dual Channel] Tag for Dual Channel Server Logs
3 participants
@naglera @ranshid @hwware