Skip to content

Conversation

@ShooterIT
Copy link
Collaborator

@ShooterIT ShooterIT commented Aug 12, 2025

  • 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

@codecov-commenter
Copy link

codecov-commenter commented Aug 12, 2025

Codecov Report

❌ Patch coverage is 83.33333% with 21 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (cluster-asm@6e71101). Learn more about missing BASE report.

Files with missing lines Patch % Lines
src/cluster_asm.c 80.80% 19 Missing ⚠️
src/networking.c 90.90% 1 Missing ⚠️
src/server.c 75.00% 1 Missing ⚠️
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           
Files with missing lines Coverage Δ
src/cluster_legacy.c 80.71% <100.00%> (ø)
src/db.c 90.89% <100.00%> (ø)
src/evict.c 96.64% <ø> (ø)
src/object.c 81.94% <100.00%> (ø)
src/replication.c 87.13% <100.00%> (ø)
src/server.h 100.00% <ø> (ø)
src/networking.c 91.31% <90.90%> (ø)
src/server.c 89.42% <75.00%> (ø)
src/cluster_asm.c 87.50% <80.80%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ShooterIT ShooterIT requested a review from tezc August 14, 2025 08:56
myself->flags |= CLUSTER_NODE_SLAVE;
clusterCloseAllSlots();
/* Cancel all ASM tasks when switching into slave */
clusterAsmCancel(NULL, "switching to replica");
Copy link
Collaborator Author

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.

Copy link
Owner

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.

Copy link
Collaborator Author

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

Copy link
Owner

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.

Copy link
Collaborator Author

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)

Copy link
Owner

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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sense

Copy link
Owner

@tezc tezc left a 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

@ShooterIT ShooterIT merged commit 5fa6730 into tezc:cluster-asm Aug 15, 2025
16 of 18 checks passed
@ShooterIT ShooterIT deleted the asm-cancel branch August 15, 2025 09:18
tezc pushed a commit that referenced this pull request Sep 10, 2025
- 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
tezc pushed a commit that referenced this pull request Sep 16, 2025
- 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants