You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "ellutionist (via GitHub)" <gi...@apache.org> on 2023/03/24 12:42:47 UTC
[GitHub] [incubator-kvrocks] ellutionist opened a new issue, #1350: Add blocking clusterx subcommand "bmigrate"
ellutionist opened a new issue, #1350:
URL: https://github.com/apache/incubator-kvrocks/issues/1350
### Search before asking
- [X] I had searched in the [issues](https://github.com/apache/incubator-kvrocks/issues) and found no similar issues.
### Motivation
For now the command to migrate slot `clusterx migrate` works asynchronously. It will return "OK" right after the migration has started. If the client wants to know the result of the migration, it has to constantly use "cluster info" to get the state.
However, once the migration has been completed, the target slot will be forbidden, and the client (cluster manager) has to actively change the topology as soon as possible, in order to make the cluster continue to work.
In summary, the current `clusterx migrate` has two shortcuts:
1. The client cannot get the result soon enough.
2. The "polling" approach to get the migration result is not efficient enough.
### Solution
I propose to leave the old `clusterx migrate` command intact, and add a new `clusterx bmirate` command, which works like other blocking commands (e.g. `blpop`). The server will block the connection and return "OK" (if succeed) or other error message (if failed) to the client only after the migration has completed.
The command shall be like:
```shell
clusterx bmirate ${SLOT} ${TARGET_NODEID} ${TIMEOUT}
```
### Are you willing to submit a PR?
- [X] I'm willing to submit a PR!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] ellutionist commented on issue #1350: Add the support of the blocking migration for the cluster migrate command
Posted by "ellutionist (via GitHub)" <gi...@apache.org>.
ellutionist commented on issue #1350:
URL: https://github.com/apache/incubator-kvrocks/issues/1350#issuecomment-1486750779
I agree with @torwig . A new subcommand is a more clear and more "redis" solution to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] xiaobiaozhao commented on issue #1350: Add blocking clusterx subcommand "bmigrate"
Posted by "xiaobiaozhao (via GitHub)" <gi...@apache.org>.
xiaobiaozhao commented on issue #1350:
URL: https://github.com/apache/incubator-kvrocks/issues/1350#issuecomment-1482761767
https://github.com/apache/incubator-kvrocks/pull/529
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on issue #1350: Add the support of the blocking migration for the cluster migrate command
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on issue #1350:
URL: https://github.com/apache/incubator-kvrocks/issues/1350#issuecomment-1492848274
> I recommend that bmirate support bulk slot migration
I think it is unrelated to this PR. Besides, it is better to implement these two different features in two separate PRs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] torwig commented on issue #1350: Add the support of the blocking migration for the cluster migrate command
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on issue #1350:
URL: https://github.com/apache/incubator-kvrocks/issues/1350#issuecomment-1484141375
I respect the reasoning about introducing a separate command for a blocking version of the command: it's clearer, descriptive and consistent with some naming conventions of Redis (e. g. POP/BPOP).
On the other hand, if I use the blocking version of the MIGRATE command by specifying the proper options, it's obvious (at least for me) that it **will block** and the OK return value means "blocking operation is completed" (in other words, migration completed).
And one quick note: personally, I don't like commands with a bunch of options (three and more). They are harder to implement and test. So, maybe _sometimes_ (but I'm not 100% sure that this case is the one), instead of introducing 2-3 additional options, we can create a new command.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] xiaobiaozhao commented on issue #1350: Add blocking clusterx subcommand "bmigrate"
Posted by "xiaobiaozhao (via GitHub)" <gi...@apache.org>.
xiaobiaozhao commented on issue #1350:
URL: https://github.com/apache/incubator-kvrocks/issues/1350#issuecomment-1482766703
I recommend that bmirate support bulk slot migration
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] ellutionist commented on issue #1350: Add blocking clusterx subcommand "bmigrate"
Posted by "ellutionist (via GitHub)" <gi...@apache.org>.
ellutionist commented on issue #1350:
URL: https://github.com/apache/incubator-kvrocks/issues/1350#issuecomment-1483980049
Thanks to @git-hulk and @xiaobiaozhao . This is a good workaround to not create a new subcommand and to provide backward compatibility.
However, I have to mention this: the "OK" returned by `migrate` without `timeout` argument means "migration started". It differs from the "OK" returned by `mirate` with `timeout` which means "migration finished". Such inconsitency may cause confusion to our users, especially who is new to Kvrocks.
If this is acceptable, then I am ready to implement it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] ShooterIT commented on issue #1350: Add the support of the blocking migration for the cluster migrate command
Posted by "ShooterIT (via GitHub)" <gi...@apache.org>.
ShooterIT commented on issue #1350:
URL: https://github.com/apache/incubator-kvrocks/issues/1350#issuecomment-1489594779
I prefer to use SYNC|ASYNC flag instead of another command which has similar arguments check and logic.
in new redis version, they don't want to add more commands, such as flush command, it has ASYNC option.
For `lpop` and `blpop`, they operate data space, different commands have different flags so redis can distinguish which command can execute in scripts env easily.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] xiaobiaozhao commented on issue #1350: Add blocking clusterx subcommand "bmigrate"
Posted by "xiaobiaozhao (via GitHub)" <gi...@apache.org>.
xiaobiaozhao commented on issue #1350:
URL: https://github.com/apache/incubator-kvrocks/issues/1350#issuecomment-1483856852
>
> Hi @ellutionist @xiaobiaozhao
>
> How about adding the optional `timeout` for the current migrate command? For example:
>
> * timeout is missing or 0 means async migration which keep the same with the current behavior
> * timeout is -1(<0) means blocking until the process was finished
> * timeout is greater than 0 means the blocking time
I agree.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on issue #1350: Add the support of the blocking migration for the cluster migrate command
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on issue #1350:
URL: https://github.com/apache/incubator-kvrocks/issues/1350#issuecomment-1487104245
Thanks, @ellutionist @torwig @xiaobiaozhao @aleksraiden.
So our consensus is to use `bmigrate` instead of adding the SYNC|ASYNC flag, it's also good to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] enjoy-binbin commented on issue #1350: Add the support of the blocking migration for the cluster migrate command
Posted by "enjoy-binbin (via GitHub)" <gi...@apache.org>.
enjoy-binbin commented on issue #1350:
URL: https://github.com/apache/incubator-kvrocks/issues/1350#issuecomment-1489645193
vote for the SYNC|ASYNC flag
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] ellutionist commented on issue #1350: Add the support of the blocking migration for the cluster migrate command
Posted by "ellutionist (via GitHub)" <gi...@apache.org>.
ellutionist commented on issue #1350:
URL: https://github.com/apache/incubator-kvrocks/issues/1350#issuecomment-1491980422
Looks like the option of `SYNC|ASYNC` flag got more votes. If there is no further advice, I will implement the new feature as following:
```
clusterx migrate ${SLOT} ${NODE_ID} [SYNC|ASYNC] ${TIMEOUT}
```
- If the `ASYNC` is given or the flag is missing, the command will act in the old way.
- If the `SYNC` flag is given:
- If the `timeout` is missing or "0", the command will block until the migration finish.
- If the `timeout` comes as a positive integer, it means the max duration (of seconds) for which the command will block.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] caipengbo commented on issue #1350: Add the support of the blocking migration for the cluster migrate command
Posted by "caipengbo (via GitHub)" <gi...@apache.org>.
caipengbo commented on issue #1350:
URL: https://github.com/apache/incubator-kvrocks/issues/1350#issuecomment-1489603672
> As I think, an ASYNC|SYNC flag is best choice and will be using in some other commands, like BACKUP.
Agree with it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on issue #1350: Add blocking clusterx subcommand "bmigrate"
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on issue #1350:
URL: https://github.com/apache/incubator-kvrocks/issues/1350#issuecomment-1483986506
@ellutionist Thanks for your points. To be clear, we can also explicitly add the ASYNC|SYNC flag to identify the mode? And the timeout is only meaningful when the SYNC flag is exists.
How do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on issue #1350: Add blocking clusterx subcommand "bmigrate"
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on issue #1350:
URL: https://github.com/apache/incubator-kvrocks/issues/1350#issuecomment-1483986662
To see other folks have any comments. @xiaobiaozhao @PragmaTwice @torwig @ShooterIT @caipengbo
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] LykxSassinator commented on issue #1350: Add the support of the blocking migration for the cluster migrate command
Posted by "LykxSassinator (via GitHub)" <gi...@apache.org>.
LykxSassinator commented on issue #1350:
URL: https://github.com/apache/incubator-kvrocks/issues/1350#issuecomment-1489641510
> I prefer to use SYNC|ASYNC flag instead of another command which has similar arguments check and logic. in new redis version, they don't want to add more commands, such as flush command, it has ASYNC option.
>
> For `lpop` and `blpop`, they operate data space, different commands have different flags so redis can distinguish which command can execute in scripts env easily.
+1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on issue #1350: Add blocking clusterx subcommand "bmigrate"
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on issue #1350:
URL: https://github.com/apache/incubator-kvrocks/issues/1350#issuecomment-1482774034
Hi @ellutionist @xiaobiaozhao
How about adding the optional `timeout` for the current migrate command?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] aleksraiden commented on issue #1350: Add the support of the blocking migration for the cluster migrate command
Posted by "aleksraiden (via GitHub)" <gi...@apache.org>.
aleksraiden commented on issue #1350:
URL: https://github.com/apache/incubator-kvrocks/issues/1350#issuecomment-1484052113
As I think, an ASYNC|SYNC flag is best choice and will be using in some other commands, like BACKUP.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice closed issue #1350: Add the support of the blocking migration for the cluster migrate command
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice closed issue #1350: Add the support of the blocking migration for the cluster migrate command
URL: https://github.com/apache/incubator-kvrocks/issues/1350
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org