fix(cluster): Support `FLUSHALL` while slot migration is in progress #3173

chakaz · 2024-06-13T12:29:45Z

Also do a small refactor to move cancellation logic into RestoreStreamer.

Fixes #3132 Also do a small refactor to move cancellation logic into `RestoreStreamer`.

BorysTheDev · 2024-06-13T13:42:54Z

src/server/journal/streamer.cc

 }

 bool RestoreStreamer::ShouldWrite(const journal::JournalItem& item) const {
+  if (item.cmd == "FLUSHALL" || item.cmd == "FLUSHDB") {


it's not only about flushALL and flushDB. We have several more global commands. You can find a list in TransactionData::IsGlobalCmd()

We have special handling for FLUSH* commands in replication as well, because these are the only commands which actually drop a db table.
Generally speaking, global commands are not supposed to be sent via migration, and so it wouldn't make sense to break a connection when we get them (it's even worse, as DFLYCLUSTER is one of those commands, so an attempt to even query the status of a migration will kill it..)

BorysTheDev · 2024-06-17T06:58:29Z

src/server/cluster/outgoing_slot_migration.cc

+void OutgoingMigration::ClearSlotRanges() {
+  shard_set->pool()->AwaitFiberOnAll([this](util::ProactorBase* pb) {
+    if (auto* shard = EngineShard::tlocal(); shard) {
+      shard->db_slice().FlushSlots(GetSlots());


why do we do flushslot for outgoing migration, but not for incoming?

that's just me being incompetent :( please ignore, thanks

@chakaz you are so far away from incompetent....

BorysTheDev · 2024-06-17T12:48:05Z

src/server/journal/streamer.cc

+  if (item.cmd == "FLUSHALL" || item.cmd == "FLUSHDB") {
+    // On FLUSH* we restart the migration
+    CHECK(dest_ != nullptr);
+    dest_->Shutdown(SHUT_RDWR);


I think we need another mechanism to stop migration. because we could have sent already FIN so we get data race in this case

I think that the use case of supporting a FLUSHALL done right with a migration finalization is not really interesting. As long as it doesn't crash - we're in a good place, consistency is pretty much bonus points
having said that, what other mechanism do you have in mind?

I think we need to provide context into restore_streamer and set error there, and set handler for error that stop migration

BorysTheDev · 2024-06-17T12:48:51Z

src/server/journal/streamer.cc

 }

 void RestoreStreamer::Start(io::AsyncSink* dest, bool send_lsn) {
+  dest_ = static_cast<util::FiberSocketBase*>(dest);


I'd prefer to provide socket or provide some context or something else but don't do static_cast

I'd want a non-down cast as well. The challenge is that if we do that, we can't override Start() (base class uses a sink)

we can provide another method, I don't see any point to repeat interface

turns out we can just replace the whole interface (including parent) to use sockets, so problem solved :)

chakaz · 2024-06-19T18:14:38Z

@BorysTheDev ready for your review 🙏

…ragonflydb#3173) * fix(cluster): Support `FLUSHALL` while slot migration is in progress Fixes dragonflydb#3132 Also do a small refactor to move cancellation logic into `RestoreStreamer`.

shahar added 2 commits June 13, 2024 15:28

fix(cluster): Support FLUSHALL while slot migration is in progress

095c2ad

Fixes #3132 Also do a small refactor to move cancellation logic into `RestoreStreamer`.

Merge branch 'main' into migration-flush

fe40946

chakaz requested a review from BorysTheDev June 13, 2024 12:41

BorysTheDev reviewed Jun 13, 2024

View reviewed changes

shahar added 2 commits June 16, 2024 13:19

Merge branch 'main' into migration-flush

515602e

fix 2 more bugs

c1e90e3

BorysTheDev reviewed Jun 17, 2024

View reviewed changes

shahar added 3 commits June 17, 2024 10:15

remove stupidness

98a7a63

more remove stupidness

ee872d5

fix

ef8aa78

chakaz requested a review from BorysTheDev June 17, 2024 11:27

Merge branch 'main' into migration-flush

c6288e6

BorysTheDev reviewed Jun 17, 2024

View reviewed changes

shahar added 2 commits June 17, 2024 23:28

fix test

c151508

no down cast

f31e8d3

chakaz requested a review from BorysTheDev June 18, 2024 07:21

shahar added 3 commits June 18, 2024 10:47

fix

3ee64e5

Merge branch 'main' into migration-flush

3f1e88c

fix occassional crash

b6dd8e6

BorysTheDev approved these changes Jun 20, 2024

View reviewed changes

chakaz merged commit f66ee5f into main Jun 20, 2024

chakaz deleted the migration-flush branch June 20, 2024 08:40

fix(cluster): Support FLUSHALL while slot migration is in progress #3173

fix(cluster): Support FLUSHALL while slot migration is in progress #3173

Uh oh!

Conversation

chakaz commented Jun 13, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chakaz commented Jun 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fix(cluster): Support `FLUSHALL` while slot migration is in progress #3173

fix(cluster): Support `FLUSHALL` while slot migration is in progress #3173