-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: add command flow for slot migration process #2292
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@BorysTheDev please add commit description to your changes. See |
40fd7cc
to
f6d8f0c
Compare
f6d8f0c
to
4af979e
Compare
712de5c
to
c8af1a5
Compare
cntx->SendOk(); | ||
} | ||
|
||
void ClusterFamily::Sync(CmdArgList args, ConnectionContext* cntx) { |
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.
Why do we need the sync sub command? cant this be part of the flow sub command?
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.
Do you mean to start sync after getting the flow command and don't wait for all flow commands?
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. I am asking.. dont know what is there reason for this. I know we have this in replication but not sure what is the reason for this in replication
RETURN_ON_ERR(SendCommandAndReadResponse(request)); | ||
|
||
PC_RETURN_ON_BAD_RESPONSE(CheckRespIsSimpleReply("OK")); | ||
|
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.
can JoinAllFlow is missing right?
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.
we do it in Destructor, I don't see why it should be here. Also, we plan not to sync flows so they can work independently, so I think joining here is superfluous,
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 think you will need a JoinAllFlow call, but probably no in this function but in the MainMigrationFb
f.e if there was error in one of the flows you will have cancel called for all flows and then you will want to join all flows before you reconnect.
Anyway we can add it in a 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.
It looks we have some misunderstanding because in this we need SYNC command from my point of view
fixes #2295 DFLYMIGRATE FLOW command was added to establish connections for every shard replication process. Slow serialization step is the separate issue so for now only eof_token is sent for reply to DFLYMIGRATE FLOW command. Expected state for START-SLOT-MIGRATION is FULL_SYNC now.
37c2f35
to
27cd6bf
Compare
fixes #2295
DFLYMIGRATE FLOW command was added to establish connections for every shard replication process.
The slow serialization step is a separate issue so for now only eof_token is sent to reply for the DFLYMIGRATE FLOW command.
The expected state for START-SLOT-MIGRATION is FULL_SYNC now.