Refactor return and goto statements #945

naglera · 2024-08-26T07:36:14Z

Consolidate the cleanup of local variables to a single point within the method, ensuring proper resource management and p
reventing memory leaks or double-free issues.

Previoslly descused here:

codecov · 2024-08-26T07:50:41Z

Codecov Report

Attention: Patch coverage is 76.47059% with 40 lines in your changes missing coverage. Please review.

Project coverage is 70.64%. Comparing base (36d438b) to head (bbc498f).
Report is 7 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/replication.c	76.47%	40 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable     #945      +/-   ##
============================================
- Coverage     70.69%   70.64%   -0.05%     
============================================
  Files           114      114              
  Lines         61693    61735      +42     
============================================
+ Hits          43611    43613       +2     
- Misses        18082    18122      +40

Files with missing lines	Coverage Δ
src/server.h	`100.00% <ø> (ø)`
src/replication.c	`87.45% <76.47%> (-0.56%)`	⬇️

... and 10 files with indirect coverage changes

ranshid · 2024-08-27T13:35:46Z

src/replication.c

@@ -3541,7 +3531,7 @@ void syncWithPrimary(connection *conn) {
            clearFailoverState();
        } else {
            abortFailover("Failover target rejected psync request");
-            return;
+            goto cleanup;


no cleanup needed in line 3524?

PingXie

I think we should consider refactoring this code further by breaking out the different if paths so each function, including fullSyncWithPrimary, is dealing with a single concern. What do you think about a skeleton function like this?

static void fullSyncWithPrimary(connection *conn) {
    char *err = NULL;

    serverAssert(conn == server.repl_rdb_transfer_s);

    if (server.repl_state == REPL_STATE_NONE) goto error;
    if (connGetState(conn) != CONN_STATE_CONNECTED) {
        serverLog(LL_WARNING, "Error condition on socket for dual channel replication: %s", connGetLastError(conn));
        goto error;
    }

    switch (server.repl_rdb_channel_state) {
        case REPL_DUAL_CHANNEL_SEND_HANDSHAKE:
            handleHandshake(conn, &err); /* TO BE FILLED OUT */
            break;
        case REPL_DUAL_CHANNEL_RECEIVE_AUTH_REPLY:
            if (server.primary_auth) {
                handleAuthReply(conn, &err);  /* TO BE FILLED OUT */
            } else {
                server.repl_rdb_channel_state = REPL_DUAL_CHANNEL_RECEIVE_REPLCONF_REPLY;
            }
            break;
        case REPL_DUAL_CHANNEL_RECEIVE_REPLCONF_REPLY:
            handleReplConfReply(conn, &err);  /* TO BE FILLED OUT */
            break;
        case REPL_DUAL_CHANNEL_RECEIVE_ENDOFF:
            handleEndOffsetResponse(conn, &err);  /* TO BE FILLED OUT */
            break;
        default:
            break;
    }

    if (err) goto error;
    return;

error:
    if (err) sdsfree(err);
    connClose(conn);
    server.repl_transfer_s = NULL;
    if (server.repl_rdb_transfer_s) {
        connClose(server.repl_rdb_transfer_s);
        server.repl_rdb_transfer_s = NULL;
    }
    if (server.repl_transfer_fd != -1) close(server.repl_transfer_fd);
    server.repl_transfer_fd = -1;
    server.repl_state = REPL_STATE_CONNECT;
    replicationAbortDualChannelSyncTransfer();
}

naglera · 2024-09-01T13:59:29Z

Started with setupMainConnForPsync and fullSyncWithPrimary. Do we also want to refactor syncWithPrimary?

PingXie

LGTM overall. My feedback is all on fit-n-finish.

PingXie · 2024-09-01T18:56:52Z

src/replication.c

+    int ret;
+
+    switch (server.repl_state) {
+    case REPL_STATE_SEND_HANDSHAKE: ret = dualChannelReplicationMainConnSendHandshake(conn, &err); break;


I like this naming scheme but can we shorten it a bit? It is a bit mouthful :)

Suggested change

case REPL_STATE_SEND_HANDSHAKE: ret = dualChannelReplicationMainConnSendHandshake(conn, &err); break;

case REPL_STATE_SEND_HANDSHAKE: ret = dualChanReplMainConnSendHandshake(conn, &err); break;

PingXie · 2024-09-01T18:57:49Z

src/replication.c

+/* Replication: Replica side.
+ * This connection handler fires after rdb-connection was initialized. We use it
+ * to adjust the replica main for loading incremental changes into the local buffer. */
+void setupMainConnForPsync(connection *conn) {


What do you think about prefixing this function with dualChan as well?

Suggested change

void setupMainConnForPsync(connection *conn) {

void dualChanSetupMainConnForPsync(connection *conn) {

Right, I just don't like the abbreviation Channel => Chan, I don't think it is very clean.

PingXie · 2024-09-01T18:58:27Z

src/replication.c

+    switch (server.repl_state) {
+    case REPL_STATE_SEND_HANDSHAKE: ret = dualChannelReplicationMainConnSendHandshake(conn, &err); break;
+    case REPL_STATE_RECEIVE_CAPA_REPLY:
+        ret = dualChannelReplicationMainConnReceiveCapaReply(conn, &err);


Suggested change

ret = dualChannelReplicationMainConnReceiveCapaReply(conn, &err);

ret = dualChanReplMainConnRecvCapaReply(conn, &err);

PingXie · 2024-09-01T18:58:46Z

src/replication.c

+        sdsfree(err);
+        err = NULL;
+        /* fall through */
+    case REPL_STATE_SEND_PSYNC: ret = dualChannelReplicationMainConnSendPsync(conn, &err); break;


Suggested change

case REPL_STATE_SEND_PSYNC: ret = dualChannelReplicationMainConnSendPsync(conn, &err); break;

case REPL_STATE_SEND_PSYNC: ret = dualChanReplMainConnSendPsync(conn, &err); break;

PingXie · 2024-09-01T18:59:20Z

src/replication.c

+        err = NULL;
+        /* fall through */
+    case REPL_STATE_SEND_PSYNC: ret = dualChannelReplicationMainConnSendPsync(conn, &err); break;
+    case REPL_STATE_RECEIVE_PSYNC_REPLY: ret = dualChannelReplicationMainConnReceivePsyncReply(conn, &err); break;


Suggested change

case REPL_STATE_RECEIVE_PSYNC_REPLY: ret = dualChannelReplicationMainConnReceivePsyncReply(conn, &err); break;

case REPL_STATE_RECEIVE_PSYNC_REPLY: ret = dualChanReplMainConnRecvPsyncReply(conn, &err); break;

PingXie · 2024-09-01T19:05:54Z

src/replication.c

@@ -55,6 +55,7 @@ int cancelReplicationHandshake(int reconnect);
 void replicationSteadyStateInit(void);
 void setupMainConnForPsync(connection *conn);
 void dualChannelSyncHandleRdbLoadCompletion(void);
+static void fullSyncWithPrimary(connection *conn);


This function is used in the dual-chan replication path only so I would think it makes more sense to prefix it too

Suggested change

static void fullSyncWithPrimary(connection *conn);

static void dualChanFullSyncWithPrimary(connection *conn);

src/replication.c

PingXie · 2024-09-01T19:16:19Z

src/replication.c

+    UNUSED(err);
+    if (server.debug_pause_after_fork) debugPauseProcess();
+    if (replicaTryPartialResynchronization(conn, 0) == PSYNC_WRITE_ERROR) {
+        serverLog(LL_WARNING, "Aborting dual channel sync. Write error.");


connection has last_errno. can we return an error string here to caller?

In the context of this specific function you are right, but generally while error strings can be useful for logging purposes, relying solely on them may not be the best approach. Based on my observations, there's no guarantee that the error string will always be non-NULL when error accures. Moreover last_errno could potentially be overridden by unrelated code changes, making it less reliable over time.

In the context of this specific function you are right, but generally while error strings can be useful for logging purposes, relying solely on them may not be the best approach.

Right. Logging is my intent here too.

there's no guarantee that the error string will always be non-NULL when error accures

I think dualChannelSetupMainConnForPsync requires a non-NULL err when C_ERR is returned - https://github.com/valkey-io/valkey/pull/945/files#diff-955c0fee32b574a1d9adb4540b8ff1564b15db9695d88a8dabc66b3c19c825daR3298

Right, fixing now

PingXie · 2024-09-01T19:17:57Z

src/replication.c

+}
+
+int dualChannelReplicationMainConnReceivePsyncReply(connection *conn, char **err) {
+    UNUSED(err);


if this function can't fail, I would suggest we null out err so we stick to a more intuitive contract, which is null err means success. Let's not leave the err state undefined (or decided by the caller).

I don't like the idea of relying on the error string. From my observation, it is not maintained for every error.

Looks like this is another inconsistency in the existing code. Functions like sendCommand return a non-NULL error string to indicate failures already. I am fine with treating an error string returned via an outgoing parameter as "informational" and for "logging purpose only" hence optional. This should help ensure consistency.

PingXie · 2024-09-01T19:22:01Z

Started with setupMainConnForPsync and fullSyncWithPrimary. Do we also want to refactor syncWithPrimary?

+1 on refactoring syncWithPrimary as well but maybe do it in a separate PR? I don't have a strong opinion though (between two PRs or a combined PRs for both syncWithPrimary and fullSyncWithPrimary).

PingXie

Thanks @naglera! I think we are very close.

src/replication.c

PingXie · 2024-09-09T03:32:13Z

src/replication.c

+        serverLog(LL_WARNING, "Sending command to primary in dual channel replication handshake: %s", *err);
+        return C_ERR;
+    }
+    server.repl_rdb_channel_state = REPL_DUAL_CHANNEL_RECEIVE_AUTH_REPLY;


I would like to see us pulling all state changes into the driver function, i.e., dualChannelFullSyncWithPrimary in this case. This way it is very easy to see all the state transitions together. I will suggest some changes in dualChannelFullSyncWithPrimary next

PingXie · 2024-09-09T03:34:21Z

src/replication.c

+    int ret;
+
+    switch (server.repl_state) {
+    case REPL_STATE_SEND_HANDSHAKE: ret = dualChannelReplMainConnSendHandshake(conn, &err); break;


This is what I meant by consolidating state manipulations in the "driver" function and then all the "worker" functions (such as dualChannelReplMainConnRecvCapaReply) become "stateless".

Suggested change

case REPL_STATE_SEND_HANDSHAKE: ret = dualChannelReplMainConnSendHandshake(conn, &err); break;

case REPL_STATE_SEND_HANDSHAKE:

ret = dualChannelReplMainConnSendHandshake(conn, &err);

if (ret != C_ERR) server.repl_state = REPL_STATE_RECEIVE_CAPA_REPLY;

/* else server.repl_state = XXX; // if it makes sense */

break;

sounds good

PingXie · 2024-09-09T03:36:21Z

src/replication.c

@@ -3415,7 +3447,7 @@ void syncWithPrimary(connection *conn) {
        if (err) goto write_error;

        server.repl_state = REPL_STATE_RECEIVE_AUTH_REPLY;
-        return;
+        goto cleanup;


Do you want to move these changes to the PR that refactors syncWithPrimary too?

yes, I will revert it

PingXie · 2024-09-09T03:38:58Z

src/replication.c

+    UNUSED(err);
+    if (server.debug_pause_after_fork) debugPauseProcess();
+    if (replicaTryPartialResynchronization(conn, 0) == PSYNC_WRITE_ERROR) {
+        serverLog(LL_WARNING, "Aborting dual channel sync. Write error.");


In the context of this specific function you are right, but generally while error strings can be useful for logging purposes, relying solely on them may not be the best approach.

Right. Logging is my intent here too.

PingXie · 2024-09-09T03:43:05Z

src/replication.c

+}
+
+int dualChannelReplicationMainConnReceivePsyncReply(connection *conn, char **err) {
+    UNUSED(err);


Looks like this is another inconsistency in the existing code. Functions like sendCommand return a non-NULL error string to indicate failures already. I am fine with treating an error string returned via an outgoing parameter as "informational" and for "logging purpose only" hence optional. This should help ensure consistency.

PingXie · 2024-09-25T00:08:54Z

src/replication.c

+    return C_OK;
+}
+
+#define C_RETRY -2


can we move this to where C_OK/C_ERR is defined?

PingXie · 2024-09-25T00:19:13Z

src/replication.c


 error:
-    sdsfree(err);
    connClose(conn);
    server.repl_transfer_s = NULL;


This looks like a bug to me. It seems the two connection objects are getting mixed up. We should zero out the RDB channel connection here, i.e., server.repl_rdb_transfer_s = NULL. Then on L2726/L2747, we should be testing the normal channel connection instead, i.e., if (server.repl_transfer_s)

Yeah we are trying to close repl_transfer_s twice 😅

Can we fix #1088 in a separate PR first? I think we will need to backport it to Valkey 8.0

src/replication.c

PingXie · 2024-09-25T01:53:58Z

src/replication.c

-        }
+    switch (server.repl_rdb_channel_state) {
+    case REPL_DUAL_CHANNEL_SEND_HANDSHAKE:
+        ret = dualChannelReplHandleHandshake(conn, &err);
        server.repl_rdb_channel_state = REPL_DUAL_CHANNEL_RECEIVE_AUTH_REPLY;


I think we should only update the state upon successful returns.

It shouldn't matter because in case of C_ERR, we will reset the state, but I agree that it makes more sense for the reader. I'll fix it.

PingXie · 2024-10-07T04:36:53Z

src/replication.c

+
+    if (connSetReadHandler(conn, dualChannelFullSyncWithPrimary) == C_ERR) {
+        char conninfo[CONN_INFO_LEN];
+        serverLog(LL_WARNING, "Can't create readable event for SYNC: %s (%s)", *err,


this seems incorrect. *err is not assigned at this point. we should revert back to strerror(errno) I think.

right err was not assigned in this case. will fix

PingXie · 2024-10-07T04:49:08Z

src/replication.c

-    sdsfree(err);
-    connClose(conn);
-    server.repl_transfer_s = NULL;
+    if (server.repl_transfer_s) {


consider adding sdsfree(err) after the error: label as well so that if we ever goto error in the future with a non-NULL err, we won't leak memory accidentally.

PingXie · 2024-10-07T04:54:11Z

src/replication.c

-        /* AUTH with the primary if required. */
+    switch (server.repl_rdb_channel_state) {
+    case REPL_DUAL_CHANNEL_SEND_HANDSHAKE:
+        ret = dualChannelReplHandleHandshake(conn, &err);


just realize that we never consume err in dualChannelFullSyncWithPrimary, whose only job when it comes to err is to free the memory. Was a logging statement missed?

You're correct, we used to log it in case of an error, but after the refactor we're already logging it inside the helper functions, so there's no need to log it inside dualChannelFullSyncWithPrimary anymore. That being said, I believe we should retain the error deallocation logic within dualChannelFullSyncWithPrimary for the sake of maintaining simplicity in our code structure.

PingXie · 2024-10-07T05:01:40Z

src/replication.c

+    UNUSED(err);
+    if (server.debug_pause_after_fork) debugPauseProcess();
+    if (replicaTryPartialResynchronization(conn, 0) == PSYNC_WRITE_ERROR) {
+        serverLog(LL_WARNING, "Aborting dual channel sync. Write error.");


there's no guarantee that the error string will always be non-NULL when error accures

I think dualChannelSetupMainConnForPsync requires a non-NULL err when C_ERR is returned - https://github.com/valkey-io/valkey/pull/945/files#diff-955c0fee32b574a1d9adb4540b8ff1564b15db9695d88a8dabc66b3c19c825daR3298

PingXie · 2024-10-07T05:03:39Z

src/replication.c

+        break;
+    default:
+        serverLog(LL_WARNING, "Unexpected replication state: %d", server.repl_state);
+        ret = C_ERR;


missing an assignment to err? This branch if hit will break the assumption on L3298.

Right, it will break the log at L3298, we should be able to handle an empty err.

Consolidate the cleanup of local variables to a single point within the method, ensuring proper resource management and preventing memory leaks or double-free issues. Signed-off-by: naglera <[email protected]>

Signed-off-by: naglera <[email protected]>

renaming and refactor Signed-off-by: naglera <[email protected]>

Co-authored-by: Ping Xie <[email protected]> Signed-off-by: Amit Nagler <[email protected]> Signed-off-by: naglera <[email protected]>

all worker functions should be stateless err should be type sds endoffset response can be empty Signed-off-by: naglera <[email protected]>

This reverts commit 3c0933497fea5d84860d24f2864a9c14ffa991bf. Signed-off-by: naglera <[email protected]>

Co-authored-by: Ping Xie <[email protected]> Signed-off-by: Amit Nagler <[email protected]> Signed-off-by: naglera <[email protected]>

Signed-off-by: naglera <[email protected]>

PingXie

I think I am running out of comments ... if you could resolve the last few comments, let's merge this PR. It is great refactoring.

PingXie · 2024-10-14T05:47:07Z

src/replication.c

+        ret = dualChannelReplHandleEndOffsetResponse(conn, &err);
+        if (ret == C_OK) server.repl_rdb_channel_state = REPL_DUAL_CHANNEL_RDB_LOAD;
+        break;
+    default: break;


if this is unreachable, we should serverPanic

PingXie · 2024-10-14T05:47:57Z

src/replication.c

+            server.repl_state = REPL_STATE_TRANSFER;
+        /*  In case the RDB is already loaded, the repl_state will be set during establishPrimaryConnection. */
+        break;
+    default:


I think it is a logic bug if we ever get here. should we serverPanic instead?

PingXie · 2024-10-14T06:00:28Z

src/replication.c

+    sdsfree(err);
+    if (ret == C_ERR) goto error;


I think we should give the error handling logic a chance to examine the err string. whether or not the error handling logic needs to consume the string is a separate concern.

Suggested change

sdsfree(err);

if (ret == C_ERR) goto error;

if (ret == C_ERR) goto error;

sdsfree(err);

Right so we should change the order to

if (ret == C_ERR) goto error; sdsfree(err);

Also maybe we better log the error string here as well just in case we didn't log it earlier (better log twice then miss it).

Signed-off-by: Amit Nagler <[email protected]>

Co-authored-by: Ping Xie <[email protected]> Signed-off-by: Amit Nagler <[email protected]> Signed-off-by: naglera <[email protected]>

Signed-off-by: naglera <[email protected]>

PingXie

LGTM. Thanks @naglera!

Consolidate the cleanup of local variables to a single point within the method, ensuring proper resource management and p reventing memory leaks or double-free issues. Previoslly descused here: - valkey-io#60 (comment) - valkey-io#60 (comment) --------- Signed-off-by: naglera <[email protected]> Signed-off-by: Amit Nagler <[email protected]> Co-authored-by: Ping Xie <[email protected]>

naglera requested a review from PingXie August 26, 2024 07:36

ranshid reviewed Aug 27, 2024

View reviewed changes

PingXie reviewed Aug 28, 2024

View reviewed changes

PingXie reviewed Sep 1, 2024

View reviewed changes

naglera mentioned this pull request Sep 3, 2024

Add [Dual Channel] Tag for Dual Channel Server Logs #986

Open

naglera force-pushed the goto-format branch from 75e5dbe to bf1fab1 Compare September 4, 2024 07:02

PingXie reviewed Sep 9, 2024

View reviewed changes

naglera force-pushed the goto-format branch from f61cf09 to 772fc62 Compare September 9, 2024 11:29

PingXie reviewed Sep 25, 2024

View reviewed changes

naglera force-pushed the goto-format branch from 003291f to 5dfb259 Compare September 29, 2024 12:00

PingXie mentioned this pull request Sep 29, 2024

[BUG] server.repl__transfer_s is double-closed/freed in fullSyncWithPrimary #1088

Closed

PingXie reviewed Oct 7, 2024

View reviewed changes

naglera force-pushed the goto-format branch from 4d5f533 to fba1983 Compare October 10, 2024 10:36

naglera and others added 12 commits October 10, 2024 10:38

Refactor return and goto statements

0bdda77

Consolidate the cleanup of local variables to a single point within the method, ensuring proper resource management and preventing memory leaks or double-free issues. Signed-off-by: naglera <[email protected]>

Refactore syncWithPrimary

3731f72

Signed-off-by: naglera <[email protected]>

refactor fullSyncWithPrimary using switch

0ab52ce

Signed-off-by: naglera <[email protected]>

refactor serupMainConn using switch

6806053

Signed-off-by: naglera <[email protected]>

Fix comments

0b8cd8f

renaming and refactor Signed-off-by: naglera <[email protected]>

Apply suggestions from code review

84637f4

Co-authored-by: Ping Xie <[email protected]> Signed-off-by: Amit Nagler <[email protected]> Signed-off-by: naglera <[email protected]>

All worker functions should be stateless

355708a

all worker functions should be stateless err should be type sds endoffset response can be empty Signed-off-by: naglera <[email protected]>

Revert "Refactore syncWithPrimary"

a5d66d2

This reverts commit 3c0933497fea5d84860d24f2864a9c14ffa991bf. Signed-off-by: naglera <[email protected]>

Apply suggestions from code review

e38603d

Co-authored-by: Ping Xie <[email protected]> Signed-off-by: Amit Nagler <[email protected]> Signed-off-by: naglera <[email protected]>

Fix comments

5707b19

Signed-off-by: naglera <[email protected]>

Fix droped text

031c07d

Signed-off-by: naglera <[email protected]>

Fix comments

611feba

Signed-off-by: naglera <[email protected]>

naglera force-pushed the goto-format branch from fba1983 to 611feba Compare October 10, 2024 10:40

PingXie reviewed Oct 14, 2024

View reviewed changes

naglera and others added 3 commits October 14, 2024 16:48

Merge branch 'unstable' into goto-format

c508802

Signed-off-by: Amit Nagler <[email protected]>

Apply suggestions from code review

683b6ac

Co-authored-by: Ping Xie <[email protected]> Signed-off-by: Amit Nagler <[email protected]> Signed-off-by: naglera <[email protected]>

Fix comments

bbc498f

Signed-off-by: naglera <[email protected]>

PingXie approved these changes Oct 15, 2024

View reviewed changes

PingXie added the run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP) label Oct 15, 2024

PingXie merged commit b0f23df into valkey-io:unstable Oct 15, 2024
47 checks passed

	case REPL_STATE_SEND_HANDSHAKE: ret = dualChannelReplicationMainConnSendHandshake(conn, &err); break;
	case REPL_STATE_SEND_HANDSHAKE: ret = dualChanReplMainConnSendHandshake(conn, &err); break;

	void setupMainConnForPsync(connection *conn) {
	void dualChanSetupMainConnForPsync(connection *conn) {

	ret = dualChannelReplicationMainConnReceiveCapaReply(conn, &err);
	ret = dualChanReplMainConnRecvCapaReply(conn, &err);

	case REPL_STATE_SEND_PSYNC: ret = dualChannelReplicationMainConnSendPsync(conn, &err); break;
	case REPL_STATE_SEND_PSYNC: ret = dualChanReplMainConnSendPsync(conn, &err); break;

	case REPL_STATE_RECEIVE_PSYNC_REPLY: ret = dualChannelReplicationMainConnReceivePsyncReply(conn, &err); break;
	case REPL_STATE_RECEIVE_PSYNC_REPLY: ret = dualChanReplMainConnRecvPsyncReply(conn, &err); break;

	static void fullSyncWithPrimary(connection *conn);
	static void dualChanFullSyncWithPrimary(connection *conn);

Refactor return and goto statements #945

Refactor return and goto statements #945

Conversation

naglera commented Aug 26, 2024

codecov bot commented Aug 26, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PingXie left a comment • edited Loading

Choose a reason for hiding this comment

naglera commented Sep 1, 2024

PingXie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PingXie commented Sep 1, 2024

PingXie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PingXie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PingXie left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 26, 2024 •

edited

Loading

PingXie left a comment •

edited

Loading