feat(replication): Do not auto replicate different master #2753

chakaz · 2024-03-20T12:31:23Z

Until now, replicas would re-connect and re-replicate a master after the master will restart. This is problematic in case the master loses its data, which will cause the replica to flush all and lose its data as well.

This is a breaking change though, in that whoever controls the replica now has to explicitly issue a REPLICAOF X Y in order to re-establish a connection to a new master. This is true even if the master loaded an up to date RDB file.

It's not necessary if the replica lost connection to the master and the master was always alive, and the connection is re-established.

Fixes #2636

Until now, replicas would re-connect and re-replicate a master after the master will restart. This is problematic in case the master loses its data, which will cause the replica to flush all and lose its data as well. This is a breaking change though, in that whoever controls the replica now has to explicitly issue a `REPLICAOF X Y` in order to re-establish a connection to a new master. This is true even if the master loaded an up to date RDB file. It's not necessary if the replica lost connection to the master and the master was always alive, and the connection is re-established. Fixes #2636

adiholden · 2024-03-21T10:50:20Z

tests/dragonfly/replication_test.py

+    await wait_available_async(c_replica)
+    assert await c_replica.execute_command("get k") == "6789"
+
+    await disconnect_clients(c_master, *[c_replica])


await disconnect_clients(c_master, c_replica)

adiholden · 2024-03-21T10:51:23Z

tests/dragonfly/replication_test.py

+
+@pytest.mark.asyncio
+async def test_replica_reconnect(df_local_factory):
+    # Connect replica to master


Please add a comment what you are checking in this test

adiholden · 2024-03-21T10:54:00Z

tests/dragonfly/replication_test.py

+    df_local_factory.start_all([master])
+
+    # Assert that replica did not reconnected to master with different repl_id
+    assert await c_master.execute_command("get k") == None


so you dont need to call master.client() after stop() ?
cause I think we do this in all our test..

nope, clients automatically reconnect

adiholden · 2024-03-21T10:55:06Z

tests/dragonfly/replication_test.py

@@ -2060,6 +2046,13 @@ async def save_replica():
    await disconnect_clients(c_master, *[c_replica])


+async def is_replica_down(conn):


Maybe rename the func name to is_replicaiton_conn_down

adiholden · 2024-03-21T11:00:14Z

tests/dragonfly/replication_test.py

@@ -1403,20 +1403,6 @@ async def test_tls_replication(
    db_size = await c_replica.execute_command("DBSIZE")
    assert 100 == db_size

-    # 4. Kill master, spin it up and see if replica reconnects


So we want to check reconnect with tls here.
Instead of removing this check lets think of another way to make the reconnect work like not using stop to break the connection but use the proxy instead

what do you mean by proxy? do we have such a thing?

take a look into test_network_disconnect test

Nice! Done.

adiholden · 2024-03-21T12:54:52Z

src/server/replica.cc

@@ -303,10 +303,17 @@ std::error_code Replica::HandleCapaDflyResp() {
  }

  // If we're syncing a different replication ID, drop the saved LSNs.
-  if (master_context_.master_repl_id != ToSV(LastResponseArgs()[0].GetBuf())) {
+  string_view master_repl_id = ToSV(LastResponseArgs()[0].GetBuf());


"This behavior change on replica side should be under flag with default to preserve the current behaviour."

I completely missed that 🤦

adiholden · 2024-03-21T13:05:46Z

tests/dragonfly/replication_test.py

+
+@pytest.mark.asyncio
+async def test_replica_reconnect(df_local_factory):
+    # Test that a replica that disconnects from a master will not automatically replicate it after connection reestablishment, if it changed repl-id.


This looks like a long line, dont we have a limit ?
'''
Test replica does not connect to master if master restarted
step1: create master and replica
step2: stop master and start again with the same port
step3: check replica is not replicating the restarted master
step4: issue new replicaof command
step5: check replica replicates master
'''

adiholden · 2024-03-21T20:07:36Z

src/server/replica.cc

@@ -41,6 +41,8 @@ ABSL_FLAG(int, master_reconnect_timeout_ms, 1000,
          "Timeout for re-establishing connection to a replication master");
 ABSL_FLAG(bool, replica_partial_sync, true,
          "Use partial sync to reconnect when a replica connection is interrupted.");
+ABSL_FLAG(bool, replica_break_on_new_master, false,


maybe replica_reconnect_on_master_restart ?

much better! (done)

adiholden · 2024-03-21T20:11:55Z

The issue #2636 says "I suggest we implement master_id protection so that replica that had been synced already and reached SSR with the master id A, won't reconnect automatically with master under the same address with master id B"
In this PR we will not reconnect if we got the repl id from master , not necessarily if we reached SSR.
I want to make sure you approve this @romange

chakaz · 2024-03-21T21:53:23Z

Thanks @adiholden, waiting for @romange's approval as well (specifically on whether to only abort replication after stable sync unlike what I did)

adiholden · 2024-03-21T21:56:36Z

tests/dragonfly/replication_test.py

@@ -32,6 +32,14 @@ async def wait_for_replicas_state(*clients, state="stable_sync", timeout=0.05):
        clients = [c for c, role in zip(clients, roles) if role[0] != "replica" or role[3] != state]


+async def close_proxy(proxy, proxy_task):


move to proxy.py

romange · 2024-03-21T22:37:20Z

Lgtm

romange · 2024-03-21T22:45:14Z

I understand the difference but I think it's manageable, the impact is that for some cases we will have a replica that does not sync. With monitoring we will be alerted of such cases. I am fine waiting to see if this becomes a priblem

chakaz added 2 commits March 20, 2024 14:00

fix test

47b7f3f

chakaz requested review from romange and adiholden March 20, 2024 19:21

adiholden reviewed Mar 21, 2024

View reviewed changes

chakaz added 2 commits March 21, 2024 13:14

fixes

235346c

proxy proxy java java

dc4c45e

chakaz requested a review from adiholden March 21, 2024 11:41

adiholden reviewed Mar 21, 2024

View reviewed changes

chakaz added 2 commits March 21, 2024 15:14

better comment

d8758a8

fix comments

5934046

adiholden reviewed Mar 21, 2024

View reviewed changes

replica_reconnect_on_master_restart

b60ffd0

adiholden previously approved these changes Mar 21, 2024

View reviewed changes

adiholden reviewed Mar 21, 2024

View reviewed changes

proxy.close()

48924fd

chakaz dismissed adiholden’s stale review via 48924fd March 24, 2024 09:13

chakaz requested a review from adiholden March 24, 2024 09:47

adiholden approved these changes Mar 24, 2024

View reviewed changes

chakaz merged commit b830a71 into main Mar 24, 2024
10 checks passed

chakaz deleted the master-replid-change branch March 24, 2024 10:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(replication): Do not auto replicate different master #2753

feat(replication): Do not auto replicate different master #2753

chakaz commented Mar 20, 2024

adiholden Mar 21, 2024

adiholden Mar 21, 2024

adiholden Mar 21, 2024

chakaz Mar 21, 2024

adiholden Mar 21, 2024

adiholden Mar 21, 2024

chakaz Mar 21, 2024

adiholden Mar 21, 2024

chakaz Mar 21, 2024

adiholden Mar 21, 2024

chakaz Mar 21, 2024

adiholden Mar 21, 2024 •

edited

Loading

adiholden Mar 21, 2024

chakaz Mar 21, 2024

adiholden commented Mar 21, 2024

chakaz commented Mar 21, 2024

adiholden Mar 21, 2024

romange commented Mar 21, 2024

romange commented Mar 21, 2024

		@@ -2060,6 +2046,13 @@ async def save_replica():
		await disconnect_clients(c_master, *[c_replica])


		async def is_replica_down(conn):

		@@ -32,6 +32,14 @@ async def wait_for_replicas_state(*clients, state="stable_sync", timeout=0.05):
		clients = [c for c, role in zip(clients, roles) if role[0] != "replica" or role[3] != state]


		async def close_proxy(proxy, proxy_task):

feat(replication): Do not auto replicate different master #2753

feat(replication): Do not auto replicate different master #2753

Conversation

chakaz commented Mar 20, 2024

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

adiholden Mar 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adiholden commented Mar 21, 2024

chakaz commented Mar 21, 2024

Choose a reason for hiding this comment

romange commented Mar 21, 2024

romange commented Mar 21, 2024

adiholden Mar 21, 2024 •

edited

Loading