feat(server): Switch to stable state replication #473

dranikpg · 2022-11-09T21:01:09Z

Migrating replication branch on main, part 6

Add basic stable state replication.

Mixed implementation. The general concept it taken from my Replication MVP.

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>

src/redis/rdb.h

dranikpg · 2022-11-09T21:02:07Z

src/server/rdb_load.h

+  std::function<void()> fullsyncb;
+


How should the loader be extended to react to events? Callback?

which events? what are you trying to do? (I have not reviewed the code yet).

to fullsync cut opcodes. The loader only finishes and unblocks once it has received the EOF code, but we need to record the FULLSYNC_CUT code earlier

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>

src/server/replica.h

dranikpg · 2022-11-13T11:21:39Z

src/server/replica.cc

-    // pass leftover data from the loader.
-    io::PrefixSource chained(loader.Leftover(), &ps);
-    VLOG(1) << "Before reading from chained stream";
-    io::Result<size_t> eof_res = chained.Read(io::MutableBytes{buf.get(), eof_token.size()});
-    if (!eof_res || *eof_res != eof_token.size()) {
+    unique_ptr<uint8_t[]> buf{new uint8_t[eof_token.size()]};
+
+    io::Result<size_t> res =
+        chained_tail.ReadAtLeast(io::MutableBytes{buf.get(), eof_token.size()}, eof_token.size());


You had a bug here: We need to use ReadAtLeast because the source might return less than n in a single call.

are you sure? based on the comments in io.h ReadAtLeast(buf, buf.size()) is equivalent to Read(buf).

Read can return less than buf.size() in a single call (see the PrefixSource read, it returns only the prefix even if its smaller than buf.size())

src/server/replica.cc

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>

src/server/replica.cc

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>

src/server/rdb_extensions.h

dranikpg · 2022-11-14T13:43:49Z

src/server/rdb_load.cc

+    auto [fit, _] = db_slice.FindExt(db_cntx, item.key);
+    if (IsValid(fit))
+      db_slice.Del(db_cntx.db_index, fit);
+
    auto [it, added] = db_slice.AddEntry(db_cntx, item.key, std::move(pv), item.expire_ms);


That's ineffective but its just a temporary stub.

The issue with AddEntry is that I give up my primevalue by moving it. In theory it should:

insert or fail

update if failed

Because looking it up separately would make the hot path a lot slower.

I've added AddOrUpdate to DbSlice as a possible solution

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>

src/server/replica.cc

dranikpg · 2022-11-14T22:18:20Z

src/server/transaction.cc

  bool lock_acquired = shard->db_slice().Acquire(mode, lock_args);
  sd.local_mask |= KEYLOCK_ACQUIRED;
-  DCHECK(!lock_acquired);  // Because CheckLock above failed.
+  // DCHECK(!lock_acquired);  // Because CheckLock above failed.


This needs a careful check: If we failed to run quickly due to the shard being locked, then this lock acquisition doesn't fail. So I commented the DCHECK out... But does the logic change in this case?

i think it's all good. you can remove this DCHECK.

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>

dranikpg · 2022-11-14T22:21:49Z

I added a line to the rdb.h file and now the ci-linter complains about formatting. Shouldn't this part of code be an exclusion?

romange

please update the commit message and the pr description accordingly.

src/server/db_slice.cc

src/server/dflycmd.cc

src/server/rdb_load.h

romange · 2022-11-16T11:45:00Z

src/server/rdb_save.cc

-    VLOG(1) << "io error " << io_error;
-    return io_error;
+  if (save_mode_ == SaveMode::SUMMARY) {
+    impl_->serializer()->SendFullSyncCut();


why do you need to send it here as well? it's not for a flow channel, right?

The io thread runs the rdb saver in SUMMARY mode to transfer only the header and lua scripts. I just decided it'll be more consistent if all threads use this opcode without any corner cases. So I need the summary mode to write is as well.

romange · 2022-11-16T11:45:30Z

src/server/rdb_save.cc

@@ -581,6 +582,11 @@ error_code RdbSerializer::SaveStreamConsumers(streamCG* cg) {
  return error_code{};
 }

+error_code RdbSerializer::SendFullSyncCut() {
+  RETURN_ON_ERR(WriteOpcode(RDB_OPCODE_FULLSYNC_END));
+  return FlushMem();


why do you need FlushMem here?

Because I want it to be sent immediately to the replica. Can't it be stuck inside the buffer if I don't flush it? It seems like it can

Does not really matter. but you call this twice: once in summary flow - it does not matter there, and the second place calls flushmem right after as far as i remember

I call it twice for the SUMMARY flow, right. But don't forget about the snapshot that sends a FS cut as well. It can be stuck there

romange · 2022-11-16T11:46:57Z

src/server/transaction.cc

  bool lock_acquired = shard->db_slice().Acquire(mode, lock_args);
  sd.local_mask |= KEYLOCK_ACQUIRED;
-  DCHECK(!lock_acquired);  // Because CheckLock above failed.
+  // DCHECK(!lock_acquired);  // Because CheckLock above failed.


i think it's all good. you can remove this DCHECK.

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>

romange · 2022-11-17T15:30:59Z

src/server/snapshot.cc

@@ -126,6 +126,9 @@ void SliceSnapshot::SerializeEntriesFb() {
  mu_.lock();
  mu_.unlock();

+  CHECK(!rdb_serializer_->SendFullSyncCut());


Because I have no error propagation... I can just remove it

Signed-off-by: Vladislav <vlad@dragonflydb.io>

dranikpg · 2022-11-17T15:39:05Z

Can I get a reply on this? 🤨 😅 #473 (comment)

romange · 2022-11-17T15:43:27Z

You've been just volunteered to check whether it's possible to exclude dirs in pre-commit checks. Инициатива наказуема!

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>

feat(server): Swith to stable state replication

5a61c39

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>

dranikpg commented Nov 9, 2022

View reviewed changes

feat(server): Full pipeline replication test

da7c901

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>

dranikpg commented Nov 13, 2022

View reviewed changes

src/server/replica.h Show resolved Hide resolved

dranikpg commented Nov 13, 2022

View reviewed changes

feat(server): Transfer leftover bytes between repl stages

e495985

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>

dranikpg force-pushed the startstable branch from 1086978 to e495985 Compare November 13, 2022 11:30

dranikpg commented Nov 13, 2022

View reviewed changes

src/server/replica.cc Outdated Show resolved Hide resolved

feat(server): Multi instance repl tests + small fixes

62b6f48

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>

dranikpg force-pushed the startstable branch from 0e08789 to 62b6f48 Compare November 13, 2022 20:48

dranikpg commented Nov 14, 2022

View reviewed changes

feat(server): Add AddOrUpdate to db slice

aa18ee2

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>

romange reviewed Nov 14, 2022

View reviewed changes

src/server/replica.cc Outdated Show resolved Hide resolved

dranikpg commented Nov 14, 2022

View reviewed changes

dranikpg marked this pull request as ready for review November 14, 2022 22:18

fix(server): Small fixes in replication code

d57a17e

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>

dranikpg force-pushed the startstable branch from ff47e04 to d57a17e Compare November 14, 2022 22:20

romange reviewed Nov 16, 2022

View reviewed changes

fix(server): Small replication code fixes

8f4ff43

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>

dranikpg requested a review from romange November 17, 2022 05:16

romange previously approved these changes Nov 17, 2022

View reviewed changes

Merge branch 'main' into startstable

44908ec

Signed-off-by: Vladislav <vlad@dragonflydb.io>

dranikpg dismissed romange’s stale review via 44908ec November 17, 2022 15:35

fix(server): Fix formatting & pre-commit hook

2acfe22

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>

dranikpg requested a review from romange November 17, 2022 16:12

romange approved these changes Nov 17, 2022

View reviewed changes

romange merged commit 96c9332 into dragonflydb:main Nov 17, 2022

dranikpg deleted the startstable branch December 25, 2022 12:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(server): Switch to stable state replication #473

feat(server): Switch to stable state replication #473

dranikpg commented Nov 9, 2022 •

edited

Loading

dranikpg Nov 9, 2022

romange Nov 14, 2022

dranikpg Nov 14, 2022

dranikpg Nov 13, 2022

romange Nov 14, 2022

dranikpg Nov 14, 2022

dranikpg Nov 14, 2022

dranikpg Nov 14, 2022

dranikpg Nov 14, 2022

romange Nov 16, 2022

dranikpg commented Nov 14, 2022

romange left a comment

romange Nov 16, 2022

dranikpg Nov 16, 2022

romange Nov 16, 2022

dranikpg Nov 16, 2022

romange Nov 17, 2022 •

edited

Loading

dranikpg Nov 17, 2022

romange Nov 16, 2022

romange Nov 17, 2022

dranikpg Nov 17, 2022

dranikpg commented Nov 17, 2022 •

edited

Loading

romange commented Nov 17, 2022

feat(server): Switch to stable state replication #473

feat(server): Switch to stable state replication #473

Conversation

dranikpg commented Nov 9, 2022 • edited Loading

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

dranikpg commented Nov 14, 2022

romange 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

romange Nov 17, 2022 • edited Loading

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

dranikpg commented Nov 17, 2022 • edited Loading

romange commented Nov 17, 2022

dranikpg commented Nov 9, 2022 •

edited

Loading

romange Nov 17, 2022 •

edited

Loading

dranikpg commented Nov 17, 2022 •

edited

Loading