Support blocking migration for the cluster migrate command #1418

ellutionist · 2023-05-04T12:45:43Z

This PR closes 1350.

ellutionist · 2023-05-04T12:48:57Z

@git-hulk @xiaobiaozhao @PragmaTwice @torwig @ShooterIT

src/commands/cmd_cluster.cc

ellutionist · 2023-05-04T14:56:29Z

@torwig @PragmaTwice The reason why I use shared_ptr of SyncMigrateContext as a field of CommandClusterX is that I have to keep the lifetime of SyncMigrateContext as long as the command object.

When the slot_migrator wake up the SyncMigrateContext with a migration result status, it has to release the pointer so that no context will "occupy" the migrator any more (see here).
However, at this time point, the WriteCB have not been triggered by the event loop yet, thus the context cannot be deconstructed yet. Using a shared pointer will bind the life cycle of the SyncMigrateContext with the CommandClusterX object, which will solve this issue.

PragmaTwice · 2023-05-04T16:22:00Z

@ellutionist Thanks for your explanation.

I have not dived into these changes deeply, but if you just want to make lifetime of SyncMigrateContext as long as the command object, you can store an instance of SyncMigrateContext in CommandClusterX directly (class CommandClusterX { SyncMigrateContext ctx_; ... }), and pass some non-owning pointer (like raw pointer, no ownership) to the migrator.

src/commands/cmd_cluster.cc

src/cluster/slot_migrate.h

xiaobiaozhao · 2023-05-05T14:24:12Z

@ellutionist Thanks for your explanation.

I have not dived into these changes deeply, but if you just want to make lifetime of SyncMigrateContext as long as the command object, you can store an instance of SyncMigrateContext in CommandClusterX directly (class CommandClusterX { SyncMigrateContext ctx_; ... }), and pass some non-owning pointer (like raw pointer, no ownership) to the migrator.

I agree with that

ellutionist · 2023-05-05T14:51:34Z

@ellutionist Thanks for your explanation.

I have not dived into these changes deeply, but if you just want to make lifetime of SyncMigrateContext as long as the command object, you can store an instance of SyncMigrateContext in CommandClusterX directly (class CommandClusterX { SyncMigrateContext ctx_; ... }), and pass some non-owning pointer (like raw pointer, no ownership) to the migrator.

@PragmaTwice @xiaobiaozhao Thank you for the advice. This is another reasonable solution. However, there are two things to note here:

The CommandClusterX does not always have a SyncMigrateContext (only when migrating with a "sync" flag). Making the context a non-pointer member of the CommandClusterX object may not make good sense here. If we decide to do this, I suggest to adopt std::optional<SyncMigrateContext>.
Due to the concern of memory safety, I am not a fan of raw pointers. Of course, in this PR we could use it, but we may set up some potential risk for the future. Because not every maintainer could get to know that "theCommandClusterX object must live long enough to complete the migration". If some change is made to "how the command objects work" in the future, it may lead to crash.

PragmaTwice · 2023-05-05T15:08:28Z

The CommandClusterX does not always have a SyncMigrateContext (only when migrating with a "sync" flag). Making the context a non-pointer member of the CommandClusterX object may not make good sense here. If we decide to do this, I suggest to adopt std::optional.

Sure, you can use something like std::optional or std::unique_ptr.

Due to the concern of memory safety, I am not a fan of raw pointers. Of course, in this PR we could use it, but we may set up some potential risk for the future. Because not every maintainer could get to know that "theCommandClusterX object must live long enough to complete the migration". If some change is made to "how the command objects work" in the future, it may lead to crash.

I think in kvrock, we use std::shared_ptr if and only if the object need to be "shared" between multiple references, and the lifetime of this object cannot be determined statically (e.g. given an object o and two reference a and b, the lifetime of o is equal to the maximum of lifetime of a and b, which need to be determined dynamically).

So currently, just "in the future ..." seems not to be an accredited reason to me, since if this reason holds, we may need to change many raw pointer in the codebase to shared_ptr. If you just need a non-owning pointer (or called an observer/view/span, in modern C++), raw pointer is currently the available choice, since std::observer_ptr or std::optional<T&> (optional with reference type) is not in the standard.

ellutionist · 2023-05-06T14:45:09Z

The CommandClusterX does not always have a SyncMigrateContext (only when migrating with a "sync" flag). Making the context a non-pointer member of the CommandClusterX object may not make good sense here. If we decide to do this, I suggest to adopt std::optional.

Sure, you can use something like std::optional or std::unique_ptr.

Due to the concern of memory safety, I am not a fan of raw pointers. Of course, in this PR we could use it, but we may set up some potential risk for the future. Because not every maintainer could get to know that "theCommandClusterX object must live long enough to complete the migration". If some change is made to "how the command objects work" in the future, it may lead to crash.

I think in kvrock, we use std::shared_ptr if and only if the object need to be "shared" between multiple references, and the lifetime of this object cannot be determined statically (e.g. given an object o and two reference a and b, the lifetime of o is equal to the maximum of lifetime of a and b, which need to be determined dynamically).

So currently, just "in the future ..." seems not to be an accredited reason to me, since if this reason holds, we may need to change many raw pointer in the codebase to shared_ptr. If you just need a non-owning pointer (or called an observer/view/span, in modern C++), raw pointer is currently the available choice, since std::observer_ptr or std::optional<T&> (optional with reference type) is not in the standard.

Thank you. I got the point of the difference between dynamic and static lifetime. From this point of view the share_ptr is not proper in this case. Perhaps the raw pointer is the best solution here.

git-hulk · 2023-05-10T03:21:25Z

Hi @ellutionist Need to use ./x.py format to correct the code format

mapleFU

Not familiar with networking logic in kvrocks, so just some style comments

mapleFU · 2023-05-10T05:15:26Z

src/cluster/sync_migrate_context.cc

+
+void SyncMigrateContext::EventCB(bufferevent *bev, int16_t events, void *ctx) {
+  auto self = reinterpret_cast<SyncMigrateContext *>(ctx);
+  auto &&slot_migrator = self->svr_->slot_migrator;


Why using auto && here?

This is a reference to std::unique_ptr<SlotMigrator>, which cannot be copied.

Technically auto& would suffice, but I personally prefer auto&& since it accepts more kinds of value (e.g. const, not this case though).

Of course, it is also okay to use auto slot_migrator = self->svr_->slot_migrator.get() (raw pointer) here. Just a choice of style.

mapleFU · 2023-05-10T05:15:35Z

src/cluster/sync_migrate_context.cc

+
+void SyncMigrateContext::TimerCB(int, int16_t events, void *ctx) {
+  auto self = reinterpret_cast<SyncMigrateContext *>(ctx);
+  auto &&slot_migrator = self->svr_->slot_migrator;


ellutionist · 2023-05-10T14:50:57Z

Hi @ellutionist Need to use ./x.py format to correct the code format

Done. It took me a while to adapt to the recent changes from [#1420].

Also some more go test cases were added.

git-hulk · 2023-05-10T15:23:07Z

The implementation generally looks good to me, left a few comments:

The blockingLock function looks unnecessary, it's fine to use std::unique_lock<std::mutex>lock(blocking_mutex_); directly
For the naming issue, SyncMigrateContext::(StartBlock|Wakeup) is a bit inconsistent with Migrator::(StartBlocking|WakeupBlocking|CancelBlocking). And for the function name, how about using Suspend|Resume|Cancel?
Those functions in SyncMigrateContext can be private:
void OnWrite(bufferevent *bev); => void onWrite(bufferevent *bev);
void OnEvent(bufferevent *bev, int16_t events); => void onEvent(bufferevent *bev, int16_t events);
void TimerCB(int, int16_t events); => void timerCB(int, int16_t events);

PragmaTwice · 2023-05-10T17:01:00Z

Seems need run ./x.py check golangci-lint.

Those functions in SyncMigrateContext can be private:

Currently we need to make them public for the CRTP base classes to use them.

git-hulk · 2023-05-11T13:23:37Z

Seems need run ./x.py check golangci-lint.

Those functions in SyncMigrateContext can be private:

Currently we need to make them public for the CRTP base classes to use them.

OK, I didn't notice this limitation.

git-hulk · 2023-05-12T02:11:00Z

Looks like the migration test case has a segment fault. https://github.com/apache/incubator-kvrocks/actions/runs/4951001502/jobs/8855626634?pr=1418

ellutionist · 2023-05-12T02:27:38Z

Looks like the migration test case has a segment fault. https://github.com/apache/incubator-kvrocks/actions/runs/4951001502/jobs/8855626634?pr=1418

I think I found the reason. When building with ASAN the pointer of context won't initialize with nullptr by default. Should explicitly assign it a default value.

PragmaTwice

LGTM

PragmaTwice · 2023-05-15T01:07:38Z

Merging... Thanks for your contribution!

support sync migration

d32acb9

add license header

ab22913

git-hulk requested review from torwig, PragmaTwice, git-hulk and xiaobiaozhao May 4, 2023 13:13

torwig reviewed May 4, 2023

View reviewed changes

src/commands/cmd_cluster.cc Outdated Show resolved Hide resolved

PragmaTwice reviewed May 4, 2023

View reviewed changes

src/commands/cmd_cluster.cc Outdated Show resolved Hide resolved

xiaobiaozhao reviewed May 5, 2023

View reviewed changes

src/commands/cmd_cluster.cc Outdated Show resolved Hide resolved

src/commands/cmd_cluster.cc Outdated Show resolved Hide resolved

src/cluster/slot_migrate.h Outdated Show resolved Hide resolved

ellutionist added 2 commits May 8, 2023 23:22

use unique_ptr instead of shared_ptr to hold SyncMigrateContext

35c0592

Improve the command args parsing

0989076

PragmaTwice changed the title ~~support sync migration~~ Support blocking migration for the cluster migrate command May 8, 2023

Merge branch 'unstable' into sync_migrate_slot

990f7dd

mapleFU reviewed May 10, 2023

View reviewed changes

ellutionist added 2 commits May 10, 2023 15:26

Format Code

2c74650

Adapt to the recent changes && add more go test cases

383dcfe

Simplify code && rename

a564974

Merge branch 'unstable' into sync_migrate_slot

4711829

initialize pointer of ctx with nullptr

5eab45e

git-hulk approved these changes May 14, 2023

View reviewed changes

git-hulk requested review from torwig, PragmaTwice, xiaobiaozhao and mapleFU May 14, 2023 08:38

Merge branch 'unstable' into sync_migrate_slot

a99f9e5

xiaobiaozhao approved these changes May 14, 2023

View reviewed changes

PragmaTwice approved these changes May 14, 2023

View reviewed changes

torwig approved these changes May 14, 2023

View reviewed changes

PragmaTwice merged commit 75db3e4 into apache:unstable May 15, 2023

infdahai mentioned this pull request May 21, 2023

migrate timeout in Go Integration Cases #1452

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support blocking migration for the cluster migrate command #1418

Support blocking migration for the cluster migrate command #1418

ellutionist commented May 4, 2023

ellutionist commented May 4, 2023

ellutionist commented May 4, 2023 •

edited

Loading

PragmaTwice commented May 4, 2023 •

edited

Loading

xiaobiaozhao commented May 5, 2023

ellutionist commented May 5, 2023 •

edited

Loading

PragmaTwice commented May 5, 2023 •

edited

Loading

ellutionist commented May 6, 2023

git-hulk commented May 10, 2023

mapleFU left a comment

mapleFU May 10, 2023

ellutionist May 10, 2023

mapleFU May 10, 2023

ellutionist commented May 10, 2023

git-hulk commented May 10, 2023 •

edited

Loading

PragmaTwice commented May 10, 2023 •

edited

Loading

git-hulk commented May 11, 2023

git-hulk commented May 12, 2023

ellutionist commented May 12, 2023

PragmaTwice left a comment

PragmaTwice commented May 15, 2023

Support blocking migration for the cluster migrate command #1418

Support blocking migration for the cluster migrate command #1418

Conversation

ellutionist commented May 4, 2023

ellutionist commented May 4, 2023

ellutionist commented May 4, 2023 • edited Loading

PragmaTwice commented May 4, 2023 • edited Loading

xiaobiaozhao commented May 5, 2023

ellutionist commented May 5, 2023 • edited Loading

PragmaTwice commented May 5, 2023 • edited Loading

ellutionist commented May 6, 2023

git-hulk commented May 10, 2023

mapleFU left a comment

Choose a reason for hiding this comment

mapleFU May 10, 2023

Choose a reason for hiding this comment

ellutionist May 10, 2023

Choose a reason for hiding this comment

mapleFU May 10, 2023

Choose a reason for hiding this comment

ellutionist commented May 10, 2023

git-hulk commented May 10, 2023 • edited Loading

PragmaTwice commented May 10, 2023 • edited Loading

git-hulk commented May 11, 2023

git-hulk commented May 12, 2023

ellutionist commented May 12, 2023

PragmaTwice left a comment

Choose a reason for hiding this comment

PragmaTwice commented May 15, 2023

ellutionist commented May 4, 2023 •

edited

Loading

PragmaTwice commented May 4, 2023 •

edited

Loading

ellutionist commented May 5, 2023 •

edited

Loading

PragmaTwice commented May 5, 2023 •

edited

Loading

git-hulk commented May 10, 2023 •

edited

Loading

PragmaTwice commented May 10, 2023 •

edited

Loading