feat(cluster): support migrate slot range

[PokIsemaine]

issues: #2355

This draft PR demonstrates how to support migrating slot ranges.

What I Did

1 Migration job - 1 slot range:

• I encapsulated a SlotRange structure and changed the migration-related class members from a single slot to a slot range.
• Reference: [NEW] Support slot-based data migration #412. Slot migration includes the following phases: start migration, migrate existing data, migrate incremental data, and end migration. In each modified phase, the entire slot range must be completed before moving to the next phase.

TODO

TODO represents the items I hope to discuss.

1. Support multiple slot ranges:

   • Current situation: Only one migration job can be performed at a time, and each migration job corresponds to a slot range.
   • Possible modification: Allow multiple migration jobs, migrating sequentially or in parallel.

2. Perform multiple migrations consecutively but do not immediately use setslot to update the topology, referring to the example in TestSlotRangeMigrate:

	t.Run("MIGRATE - Repeat migration cases, but does not immediately update the topology via setslot", func(t *testing.T) {
		// Disjoint
		require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", "114-116", id1).Val())
		waitForMigrateSlotRangeState(t, rdb0, "114-116", SlotMigrationStateSuccess)
		require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", "117-118", id1).Val())
		waitForMigrateSlotRangeState(t, rdb0, "117-118", SlotMigrationStateSuccess)
		require.Equal(t, "OK", rdb0.Do(ctx, "clusterx", "migrate", "112-113", id1).Val())
		waitForMigrateSlotRangeState(t, rdb0, "112-113", SlotMigrationStateSuccess)

		errMsg := "Can't migrate slot which has been migrated"
		// TODO: Migrating 112-113, but 114-118 is covered and cannot be detected.
		// require.ErrorContains(t, rdb0.Do(ctx, "clusterx", "migrate", "114-116", id1).Err(), errMsg)
		// require.ErrorContains(t, rdb0.Do(ctx, "clusterx", "migrate", "117-118", id1).Err(), errMsg)

		// Intersection
		require.ErrorContains(t, rdb0.Do(ctx, "clusterx", "migrate", "112", id1).Err(), errMsg)
		require.ErrorContains(t, rdb0.Do(ctx, "clusterx", "migrate", "112-112", id1).Err(), errMsg)
		require.ErrorContains(t, rdb0.Do(ctx, "clusterx", "migrate", "113", id1).Err(), errMsg)
		require.ErrorContains(t, rdb0.Do(ctx, "clusterx", "migrate", "113-113", id1).Err(), errMsg)

		// Subset
		require.ErrorContains(t, rdb0.Do(ctx, "clusterx", "migrate", "112-113", id1).Err(), errMsg)
		require.ErrorContains(t, rdb0.Do(ctx, "clusterx", "migrate", "112-120", id1).Err(), errMsg)
		require.ErrorContains(t, rdb0.Do(ctx, "clusterx", "migrate", "110-112", id1).Err(), errMsg)
	})

This situation also seems to exist in the original single slot migration, and I am not sure if such operations are reasonable.

migrate A => migrate B => setslot A&B
migrate A => migrate B => migrate A (expected to fail, but allowed to pass) => setslot A&B

// slotrange A-C
migrate A-B => migrate C => setslot A-C
migrate A-B => migrate C => migrate B (expected error, but allowed to pass) => setslot A-C

3. More precise migration failure slot range:
   The current implementation determines the entire slot range to fail and cleans it up, and the user later migrates the entire slot range again.
   Do we want to support a more precise failure range? For example, [start_slot-fail_slot), [fail_slot, end]. Users can check the status with commands such as cluster info and then re-migrate the failed slot range by themselves. (Personally, I think it's a bit cumbersome and error-prone)

Miscellaneous

Other suggestions are welcome, such as more testing, code optimization, better user interaction, etc.

[git-hulk]

@PokIsemaine Thanks for your impressive contributions. I will take another pass while it's ready to be reviewed.

[PokIsemaine]

@git-hulk @caipengbo

If it's convenient, I would like to discuss some matters related to the TODO list:

First, regarding item 2 on the TODO list, I find this situation a bit confusing because I am not sure if this is the intended design of kvrocks. If possible, please determine whether this is a normal situation.

Second, concerning items 1 and 3 on the TODO list, these are enhancements. Please evaluate whether these features are needed and if they should be included in this PR.

[caipengbo]

@PokIsemaine

First, regarding item 2 on the TODO list, I find this situation a bit confusing because I am not sure if this is the intended design of kvrocks. If possible, please determine whether this is a normal situation.

This is indeed the case at the moment. The migrated slots is not checked when the migration is performed, and I think they should be checked.

Second, concerning items 1 and 3 on the TODO list, these are enhancements. Please evaluate whether these features are needed and if they should be included in this PR.

I think these are good improvements, but we can add them in the subsequent PR. This PR is enough.

[PokIsemaine]

@caipengbo Ok, thanks for the suggestion. So for the second TODO, do we try to fix it at this PR or create another separate issue to track it?

[caipengbo]

for the second TODO, do we try to fix it at this PR or create another separate issue to track it?

Let's fix it in this PR. @PokIsemaine

[git-hulk]

@PokIsemaine I think this PR is ready to review now?

[PokIsemaine]

@PokIsemaine I think this PR is ready to review now?

You can start the review now :)

The clang-tidy check in the CI doesn't seem to be working, so please try to re-run it. This works in my local and fork repository CI.

[PragmaTwice]

Could you fix these clang-tidy issues? Please make sure the code can be successfully compiled.

/home/runner/work/kvrocks/kvrocks/build/_deps/fmt-src/include/fmt/base.h:1619:3: error: static_assert failed due to requirement 'formattable_pointer' "Formatting of non-void pointers is disallowed." [clang-diagnostic-error]
Suppressed 21102 warnings (21069 in non-user code, 32 NOLINT, 1 with check filters).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
Found compiler error(s).
  static_assert(formattable_pointer,
  ^             ~~~~~~~~~~~~~~~~~~~
/home/runner/work/kvrocks/kvrocks/build/_deps/fmt-src/include/fmt/base.h:2002:20: note: in instantiation of function template specialization 'fmt::detail::make_arg<true, fmt::context, double (double) noexcept, 0>' requested here
  return {{detail::make_arg<NUM_ARGS <= detail::max_packed_args, Context>(
                   ^
/home/runner/work/kvrocks/kvrocks/build/_deps/fmt-src/include/fmt/format.h:4357:28: note: in instantiation of function template specialization 'fmt::make_format_args<fmt::context, double (double) noexcept, std::basic_string<char>, 2UL, 0UL, 223ULL, 0>' requested here
  return vformat(fmt, fmt::make_format_args(args...));
                           ^
/home/runner/work/kvrocks/kvrocks/src/cluster/slot_migrate.cc:394:33: note: in instantiation of function template specialization 'fmt::format<double (&)(double) noexcept, std::basic_string<char> &>' requested here
    return {Status::NotOK, fmt::format("failed to iterate keys of slot {}: {}", slot, err_str)};
                                ^
/home/runner/work/kvrocks/kvrocks/src/cluster/slot_migrate.cc:393:66: error: use of undeclared identifier 'slot' [clang-diagnostic-error]
    LOG(ERROR) << "[migrate] Failed to iterate keys of slot " << slot << ": " << err_str;
                                                                 ^
/home/runner/work/kvrocks/kvrocks/src/cluster/slot_migrate.cc:394:81: error: use of undeclared identifier 'slot' [clang-diagnostic-error]
    return {Status::NotOK, fmt::format("failed to iterate keys of slot {}: {}", slot, err_str)};
                                                                                ^

[git-hulk]

@PokIsemaine I think this PR is ready to review now?

You can start the review now :)

The clang-tidy check in the CI doesn't seem to be working, so please try to re-run it. This works in my local and fork repository CI.

Thank you! I will take another pass in one or two days.

[git-hulk]

This PR generally looks good to me, left two comments. To see if other maintainers have further comments.

[mapleFU]

+1 for we with some nits

[git-hulk]

Will merge this PR if no further comments and CI is passed.

[sonarcloud]

Quality Gate passed

Issues
26 New issues
0 Accepted issues

Measures
0 Security Hotspots
59.5% Coverage on New Code
0.2% Duplication on New Code

See analysis details on SonarCloud

[mapleFU]

Nice work, thanks!