-
Notifications
You must be signed in to change notification settings - Fork 0
some improvements for slot migration #55
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
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## cluster-asm #55 +/- ##
==============================================
Coverage ? 69.63%
==============================================
Files ? 125
Lines ? 73777
Branches ? 0
==============================================
Hits ? 51376
Misses ? 22401
Partials ? 0
🚀 New features to boost your workflow:
|
| myself->flags |= CLUSTER_NODE_SLAVE; | ||
| clusterCloseAllSlots(); | ||
| /* Cancel all ASM tasks when switching into slave */ | ||
| clusterAsmCancel(NULL, "switching to replica"); |
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.
Hi @tezc not sure if the cluster plugin has this function to allow us to cancel asm tasks
if not, we can check the change of role in asmBeforeSleep, or other places, but it looks like ugly.
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 see we send ASM_EVENT_FAILED event but we can also send ASM_EVENT_CANCELLED to indicate the task is cancelled. I can handle it later.
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.
no, i mean do we have a similar clusterSetMaster function, switching a master to replica
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.
oh I see. I'll check with Josh later. Plugin may cancel itself but perhaps it is always a good idea that redis should be notified whenever there is a config change like this.
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.
plugin should cancel asm in such cases:
- change slots directly
- switch master to slave
- delete cluster node (i think i will fix in next 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 remember we talked about this before, plugin would call us to notify on config change but not yet implemented. Perhaps, plugin may cancel externally but we can also internally cancel it on this notification (just to avoid ugly scenarios in case there is a bug in plugin)
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.
make sense
tezc
left a comment
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.
thanks, this PR is going to cleanup lots of mess
- flush-like command will cancel slot migration task - switching to replica from master will cancel slot migration task - resolve the source node when starting importing tasks - hide rdb/main channel client in info replication - in client list, migrating client is marked as m, importing client is marked as i - show asm_migrating_buffer and asm_importing_buffer in info memory - reject cluster setslot if there is an active slot migration, and cancel tasks if slot topology is changed
- flush-like command will cancel slot migration task - switching to replica from master will cancel slot migration task - resolve the source node when starting importing tasks - hide rdb/main channel client in info replication - in client list, migrating client is marked as m, importing client is marked as i - show asm_migrating_buffer and asm_importing_buffer in info memory - reject cluster setslot if there is an active slot migration, and cancel tasks if slot topology is changed
info replicationclient list, migrating client is marked asm, importing client is marked asiasm_migrating_bufferandasm_importing_bufferininfo memory