-
Notifications
You must be signed in to change notification settings - Fork 58
feat(disk_balance): add do_disk_migrate_replica
to support migrate origin data
#664
Conversation
return; | ||
} | ||
|
||
if (init_target_dir(req) && migrate_replica_checkpoint(req) && migrate_replica_app_info(req)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This entire execution spends too long, it will probably block other tasks on the REPLICATION_LONG thread pool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, but the process need spend the long time, if not allow block REPLICATION_LONG
, put DEFAULT
?
if (utils::filesystem::directory_exists(replica_dir)) { | ||
derror_replica("migration target replica dir({}) has existed", replica_dir); | ||
reset_status(); | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So how does the user know the result that failure happened?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copy data is designed async and no response the status, the result
now need check the log
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or I can add query_status
rpc in later pr?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @neverchanje, there should be a way to let user notice failure happened, query_status
rpc may be helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add it in later pr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what is your plan? You certainly can not actively notify the user. One possible way is to save the error, and every time user queries the status, if failure happened, you respond with that error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, now has one way is run disk-migrate -g gpid -n node
(not add other argument), if has one task is running, it will show:
Internal server error [DiskMigrate to session 127.0.0.1:45801(replica)] failed: ErrorCode({Errno:ERR_BUSY}):Existed migrate task(replication::disk_migration_status::MOVED) is running]
We can get the task process but not need query_status
, I think we can use this first, if need query
, I can add it in later pr
|
||
// _target_replica_data_dir = /root/gpid.app_type.disk.balance.tmp/data/rdb, it will update to | ||
// /root/target/gpid.app_type/data/rdb in replica_disk_migrator::update_replica_dir finally | ||
_target_data_dir = utils::filesystem::path_combine(_target_replica_dir, kDataDirFolder); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are _target_replica_dir = fmt::format("{}{}", replica_dir, kReplicaDirTempSuffix);
and _target_data_dir = utils::filesystem::path_combine(_target_replica_dir, kDataDirFolder);
the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
filesystem::path_combine("/abc", "file") => /abc/file, but format("/abc", "file")=>/abcfile
void replica_disk_migrator::migrate_replica(const replica_disk_migrate_request &req) {} | ||
void replica_disk_migrator::migrate_replica(const replica_disk_migrate_request &req) | ||
{ | ||
if (status() != disk_migration_status::MOVING) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When will this condition happen? could you please take an example for me?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now hasn't this condition, add it only make sure the process must under MOVING
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I suggest that you can consider this condition in your further pull request.
if (utils::filesystem::directory_exists(replica_dir)) { | ||
derror_replica("migration target replica dir({}) has existed", replica_dir); | ||
reset_status(); | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @neverchanje, there should be a way to let user notice failure happened, query_status
rpc may be helpful.
RPC_REPLICA_DISK_MIGRATE
) and check whether the request is valid feat(disk_balance): support and validate disk migration rpc #660.do_disk_migrate_replica
to support migrate origin datareplica migration status
#660 complete the migration request args check, this pr add
do_disk_migrate_replica
and support migrate origin data. The migration status fromMOVING
toMOVED