Stabilize dual replication test to avoid getting LOADING error by enjoy-binbin · Pull Request #1288 · valkey-io/valkey

enjoy-binbin · 2024-11-11T05:21:14Z

When doing $replica replicaof no one, we may get a LOADING
error, this is because during the test execution, the replica
may reconnect very quickly, and the full sync is initiated,
and the replica has entered the LOADING state.

In this commit, we make sure the primary is pasued after the
fork, so the replica won't enter the LOADING state, and with
this fix, this test seems more natural and predictable.

When doing `$replica replicaof no one`, we may get a LOADING error, this is because during the test execution, the replica may reconnect very quickly, and the full sync is initiated, and the replica has entered the LOADING state. In this commit, we make sure the primary is pasued after the fork, so the replica won't enter the LOADING state, and with this fix, this test seems more natural and predictable. Signed-off-by: Binbin <binloveplay1314@qq.com>

enjoy-binbin · 2024-11-11T05:22:14Z

Failed CI link: https://github.com/valkey-io/valkey/actions/runs/11756610760/job/32752932086?pr=1009#step:5:5820. Mentioned in here: #1009 (comment)

codecov · 2024-11-11T05:35:25Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.70%. Comparing base (45d596e) to head (5afd936).
Report is 3 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #1288      +/-   ##
============================================
+ Coverage     70.56%   70.70%   +0.13%     
============================================
  Files           114      114              
  Lines         63147    63154       +7     
============================================
+ Hits          44560    44653      +93     
+ Misses        18587    18501      -86

see 17 files with indirect coverage changes

naglera · 2024-11-11T08:33:18Z

LGTM.
I previously believed that the replicaof command would be processed even during RDB loading. However, upon examining the code more closely, I see that it is actually disallowed when the loading process is asynchronous, such as in the case of dual-channel synchronization.

zuiderkwast

Makes sense. It's a complicated test case, but it seems we need this pause to stabilize it.

Signed-off-by: Binbin <binloveplay1314@qq.com>

enjoy-binbin · 2024-11-12T07:18:41Z

@naglera https://github.com/valkey-io/valkey/actions/runs/11788187416/job/32834861441#step:6:6097

Today we have a CI failure, it look like i introduced it.

The reason is that, since i added a pause, the replica will reconnect, and the rdb channel is connected. And then the rdb child is drop, and we will postpone the rdb client.

And then in here, we will wait a client_output_buffer_limit_disconnections, and we got an unexpected rdb client, the previous one, so the test fail.

            wait_for_condition 500 1000 {
                [s -1 client_output_buffer_limit_disconnections] > $cur_client_closed_count
            } else {
                fail "Primary should disconnect replica due to COB overrun"
            }

i was wondering in here, in this case, should we still want to postpone the rdb client? In this case, it seems like we no longer need the rdb client since the rdb child is dead.

void updateReplicasWaitingBgsave(int bgsaveerr, int type) {
    listNode *ln;
    listIter li;

    /* Note: there's a chance we got here from within the REPLCONF ACK command
     * so we must avoid using freeClient, otherwise we'll crash on our way up. */

    listRewind(server.replicas, &li);
    while ((ln = listNext(&li))) {
        client *replica = ln->value;

        if (replica->repl_state == REPLICA_STATE_WAIT_BGSAVE_END) {
            int repldbfd;
            struct valkey_stat buf;

            if (bgsaveerr != C_OK) {
+                if (replica->flag.protected_rdb_channel) {
+                    replica->flag.protected_rdb_channel = 0;
+                    // removeReplicaFromPsyncWait(replica);
+                    serverLog(xxx)
+                }
                freeClientAsync(replica);
                serverLog(LL_WARNING, "SYNC failed. BGSAVE child returned an error");
                continue;
            }

i ran a daily test with --single and --loops 10, the test passed, so the fix seems solid. daily test: https://github.com/enjoy-binbin/valkey/actions/runs/11810068690

…y-io#1288) When doing `$replica replicaof no one`, we may get a LOADING error, this is because during the test execution, the replica may reconnect very quickly, and the full sync is initiated, and the replica has entered the LOADING state. In this commit, we make sure the primary is pasued after the fork, so the replica won't enter the LOADING state, and with this fix, this test seems more natural and predictable. Signed-off-by: Binbin <binloveplay1314@qq.com>

artikell · 2025-04-18T15:03:53Z

@enjoy-binbin This problem occurs again, maybe you can take a look.

https://github.com/artikell/valkey/actions/runs/14535230702/job/4078222933

Supplement the repair of #1288, need to wait for the Loading to complete before executing `$replica replicaof no one`. Signed-off-by: artikell <739609084@qq.com>

…y-io#1979) Supplement the repair of valkey-io#1288, need to wait for the Loading to complete before executing `$replica replicaof no one`. Signed-off-by: artikell <739609084@qq.com> Signed-off-by: Nitai Caro <caronita@amazon.com>

…y-io#1979) Supplement the repair of valkey-io#1288, need to wait for the Loading to complete before executing `$replica replicaof no one`. Signed-off-by: artikell <739609084@qq.com>

…y-io#1979) Supplement the repair of valkey-io#1288, need to wait for the Loading to complete before executing `$replica replicaof no one`. Signed-off-by: artikell <739609084@qq.com> Signed-off-by: hwware <wen.hui.ware@gmail.com>

…y-io#1288) When doing `$replica replicaof no one`, we may get a LOADING error, this is because during the test execution, the replica may reconnect very quickly, and the full sync is initiated, and the replica has entered the LOADING state. In this commit, we make sure the primary is pasued after the fork, so the replica won't enter the LOADING state, and with this fix, this test seems more natural and predictable. Signed-off-by: Binbin <binloveplay1314@qq.com>

…y-io#1288) When doing `$replica replicaof no one`, we may get a LOADING error, this is because during the test execution, the replica may reconnect very quickly, and the full sync is initiated, and the replica has entered the LOADING state. In this commit, we make sure the primary is pasued after the fork, so the replica won't enter the LOADING state, and with this fix, this test seems more natural and predictable. Signed-off-by: Binbin <binloveplay1314@qq.com> (cherry picked from commit 82d551d)

…y-io#1979) Supplement the repair of valkey-io#1288, need to wait for the Loading to complete before executing `$replica replicaof no one`. Signed-off-by: artikell <739609084@qq.com> (cherry picked from commit 0b94ca6)

…y-io#1288) When doing `$replica replicaof no one`, we may get a LOADING error, this is because during the test execution, the replica may reconnect very quickly, and the full sync is initiated, and the replica has entered the LOADING state. In this commit, we make sure the primary is pasued after the fork, so the replica won't enter the LOADING state, and with this fix, this test seems more natural and predictable. Signed-off-by: Binbin <binloveplay1314@qq.com> (cherry picked from commit 82d551d)

When doing `$replica replicaof no one`, we may get a LOADING error, this is because during the test execution, the replica may reconnect very quickly, and the full sync is initiated, and the replica has entered the LOADING state. In this commit, we make sure the primary is pasued after the fork, so the replica won't enter the LOADING state, and with this fix, this test seems more natural and predictable. Signed-off-by: Binbin <binloveplay1314@qq.com> (cherry picked from commit 82d551d)

…#1979) Supplement the repair of valkey-io/valkey#1288, need to wait for the Loading to complete before executing `$replica replicaof no one`. Signed-off-by: artikell <739609084@qq.com>

enjoy-binbin requested a review from naglera November 11, 2024 05:21

enjoy-binbin mentioned this pull request Nov 11, 2024

Fix replica not able to initate election in time when epoch fails #1009

Merged

naglera approved these changes Nov 11, 2024

View reviewed changes

enjoy-binbin requested a review from zuiderkwast November 11, 2024 08:38

xbasel approved these changes Nov 11, 2024

View reviewed changes

zuiderkwast reviewed Nov 11, 2024

View reviewed changes

Comment thread tests/integration/dual-channel-replication.tcl Outdated

zuiderkwast changed the title ~~Stable dual replication test to avoid getting LOADING error~~ Stabilize dual replication test to avoid getting LOADING error Nov 11, 2024

update proc name

5afd936

Signed-off-by: Binbin <binloveplay1314@qq.com>

zuiderkwast approved these changes Nov 11, 2024

View reviewed changes

enjoy-binbin merged commit 4aacffa into valkey-io:unstable Nov 11, 2024

enjoy-binbin deleted the fix_test branch November 11, 2024 13:42

enjoy-binbin mentioned this pull request Nov 13, 2024

Unprotect rdb channel when bgsave child fails in dual channel replication #1297

Merged

artikell mentioned this pull request Apr 19, 2025

Stabilize dual replication test to avoid getting LOADING error #1979

Merged

sarthakaggarwal97 mentioned this pull request May 6, 2026

Backport Test Fixes 8.0 #3638

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Stabilize dual replication test to avoid getting LOADING error#1288

Stabilize dual replication test to avoid getting LOADING error#1288
enjoy-binbin merged 2 commits into
valkey-io:unstablefrom
enjoy-binbin:fix_test

enjoy-binbin commented Nov 11, 2024

Uh oh!

enjoy-binbin commented Nov 11, 2024 •

edited

Loading

Uh oh!

codecov Bot commented Nov 11, 2024 •

edited

Loading

Uh oh!

naglera commented Nov 11, 2024

Uh oh!

zuiderkwast left a comment

Uh oh!

Uh oh!

enjoy-binbin commented Nov 12, 2024 •

edited

Loading

Uh oh!

artikell commented Apr 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

enjoy-binbin commented Nov 11, 2024

Uh oh!

enjoy-binbin commented Nov 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Nov 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

naglera commented Nov 11, 2024

Uh oh!

zuiderkwast left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

enjoy-binbin commented Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

artikell commented Apr 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

enjoy-binbin commented Nov 11, 2024 •

edited

Loading

codecov Bot commented Nov 11, 2024 •

edited

Loading

enjoy-binbin commented Nov 12, 2024 •

edited

Loading