diff --git a/src/cluster_migrateslots.c b/src/cluster_migrateslots.c index 5ad01b3d2ee..4c1951e9d36 100644 --- a/src/cluster_migrateslots.c +++ b/src/cluster_migrateslots.c @@ -1448,8 +1448,8 @@ int slotExportTryDoPause(slotMigrationJob *job) { return C_ERR; } serverLog(LL_NOTICE, - "Pausing writes to allow slot migration %s to finalize failover.", - job->description); + "Pausing writes (remaining_repl_size is %lld) to allow slot migration %s to finalize failover.", + job->client->reply_bytes, job->description); job->mf_end = mstime() + server.cluster_mf_timeout * CLUSTER_MF_PAUSE_MULT; pauseActions(PAUSE_DURING_SLOT_MIGRATION, job->mf_end, PAUSE_ACTIONS_CLIENT_WRITE_SET); diff --git a/src/config.c b/src/config.c index 48ea1c46961..e40c24b5d8f 100644 --- a/src/config.c +++ b/src/config.c @@ -2168,6 +2168,13 @@ static int numericParseString(standardConfig *config, sds value, const char **er int memerr; *res = memtoull(value, &memerr); if (!memerr) return 1; + + /* memtoull rejects negative values, but some memory configs accept + * special negative values (e.g. -1 to disable a limit). Fall back + * to plain integer parsing for those. */ + if (config->data.numeric.flags & SIGNED_MEMORY_CONFIG) { + if (string2ll(value, sdslen(value), res)) return 1; + } } /* Attempt to parse as percent */ @@ -2241,6 +2248,8 @@ static sds numericConfigGet(standardConfig *config) { int len = ll2string(buf, sizeof(buf), -value); buf[len] = '%'; buf[len + 1] = '\0'; + } else if (config->data.numeric.flags & SIGNED_MEMORY_CONFIG && value < 0) { + ll2string(buf, sizeof(buf), value); } else if (config->data.numeric.flags & MEMORY_CONFIG) { ull2string(buf, sizeof(buf), value); } else if (config->data.numeric.flags & OCTAL_CONFIG) { @@ -2260,6 +2269,8 @@ static void numericConfigRewrite(standardConfig *config, const char *name, struc if (config->data.numeric.flags & PERCENT_CONFIG && value < 0) { rewriteConfigPercentOption(state, name, -value, config->data.numeric.default_value); + } else if (config->data.numeric.flags & SIGNED_MEMORY_CONFIG && value < 0) { + rewriteConfigNumericalOption(state, name, value, config->data.numeric.default_value); } else if (config->data.numeric.flags & MEMORY_CONFIG) { rewriteConfigBytesOption(state, name, value, config->data.numeric.default_value); } else if (config->data.numeric.flags & OCTAL_CONFIG) { @@ -3452,7 +3463,7 @@ standardConfig static_configs[] = { createSizeTConfig("tracking-table-max-keys", NULL, MODIFIABLE_CONFIG, 0, LONG_MAX, server.tracking_table_max_keys, 1000000, INTEGER_CONFIG, NULL, NULL), /* Default: 1 million keys max. */ createSizeTConfig("client-query-buffer-limit", NULL, DEBUG_CONFIG | MODIFIABLE_CONFIG, 1024 * 1024, LONG_MAX, server.client_max_querybuf_len, 1024 * 1024 * 1024, MEMORY_CONFIG, NULL, NULL), /* Default: 1GB max query buffer. */ createSSizeTConfig("maxmemory-clients", NULL, MODIFIABLE_CONFIG, -100, SSIZE_MAX, server.maxmemory_clients, 0, MEMORY_CONFIG | PERCENT_CONFIG, NULL, applyClientMaxMemoryUsage), - createSSizeTConfig("slot-migration-max-failover-repl-bytes", NULL, MODIFIABLE_CONFIG, -1, SSIZE_MAX, server.slot_migration_max_failover_repl_bytes, 0, MEMORY_CONFIG, NULL, NULL), + createSSizeTConfig("slot-migration-max-failover-repl-bytes", NULL, MODIFIABLE_CONFIG, -1, SSIZE_MAX, server.slot_migration_max_failover_repl_bytes, 0, MEMORY_CONFIG | SIGNED_MEMORY_CONFIG, NULL, NULL), /* Other configs */ createTimeTConfig("repl-backlog-ttl", NULL, MODIFIABLE_CONFIG, 0, LONG_MAX, server.repl_backlog_time_limit, 60 * 60, INTEGER_CONFIG, NULL, NULL), /* Default: 1 hour */ @@ -3519,6 +3530,18 @@ void initConfigValues(void) { dictExpand(configs, sizeof(static_configs) / sizeof(standardConfig)); for (standardConfig *config = static_configs; config->name != NULL; config++) { if (config->interface.init) config->interface.init(config); + + if (config->type == NUMERIC_CONFIG) { + /* SIGNED_MEMORY_CONFIG must be used together with MEMORY_CONFIG. */ + serverAssert(!(config->data.numeric.flags & SIGNED_MEMORY_CONFIG) || + (config->data.numeric.flags & MEMORY_CONFIG)); + + /* PERCENT_CONFIG and SIGNED_MEMORY_CONFIG both use negative values + * with different semantics, so they must not be combined. */ + serverAssert(!((config->data.numeric.flags & PERCENT_CONFIG) && + (config->data.numeric.flags & SIGNED_MEMORY_CONFIG))); + } + /* Add the primary config to the dictionary. */ int ret = registerConfigValue(config->name, config, 0); serverAssert(ret); diff --git a/src/server.h b/src/server.h index d28ec2129e0..b84858a2f18 100644 --- a/src/server.h +++ b/src/server.h @@ -3640,11 +3640,13 @@ sds keyspaceEventsFlagsToString(int flags); * to apply the configuration change even if the new config value is the same as \ * the old. */ -#define INTEGER_CONFIG 0 /* No flags means a simple integer configuration */ -#define MEMORY_CONFIG (1 << 0) /* Indicates if this value can be loaded as a memory value */ -#define PERCENT_CONFIG (1 << 1) /* Indicates if this value can be loaded as a percent (and stored as a negative int) */ -#define OCTAL_CONFIG (1 << 2) /* This value uses octal representation */ -#define UNSIGNED_CONFIG (1 << 3) /* This value uses unsigned representation */ +/* Numeric Flags */ +#define INTEGER_CONFIG 0 /* No flags means a simple integer configuration */ +#define MEMORY_CONFIG (1 << 0) /* Indicates if this value can be loaded as a memory value */ +#define PERCENT_CONFIG (1 << 1) /* Indicates if this value can be loaded as a percent (and stored as a negative int) */ +#define OCTAL_CONFIG (1 << 2) /* This value uses octal representation */ +#define UNSIGNED_CONFIG (1 << 3) /* This value uses unsigned representation */ +#define SIGNED_MEMORY_CONFIG (1 << 4) /* A MEMORY_CONFIG that also accepts plain negative integers */ /* Enum Configs contain an array of configEnum objects that match a string with an integer. */ typedef struct configEnum { diff --git a/tests/unit/cluster/cluster-migrateslots.tcl b/tests/unit/cluster/cluster-migrateslots.tcl index 4137702346f..767351bf9ec 100644 --- a/tests/unit/cluster/cluster-migrateslots.tcl +++ b/tests/unit/cluster/cluster-migrateslots.tcl @@ -2450,7 +2450,7 @@ start_cluster 3 0 {tags {logreqres:skip external:skip cluster} overrides {cluste set 16383_slot_tag "{6ZJ}" set_debug_prevent_pause 1 - # Move 16383 from R0 to R2 + # Move 16383 from R2 to R0. assert_match "OK" [R 2 CLUSTER MIGRATESLOTS SLOTSRANGE 16383 16383 NODE [R 0 CLUSTER MYID]] set jobname [get_job_name 2 16383] wait_for_migration_field 2 $jobname state waiting-to-pause @@ -2482,5 +2482,48 @@ start_cluster 3 0 {tags {logreqres:skip external:skip cluster} overrides {cluste set export_migration [get_migration_by_name 2 $jobname] assert_equal [dict get $export_migration remaining_repl_size] 0 assert_equal [dict get $import_migration remaining_repl_size] 0 + + # Check the remaining_repl_size log is OK. + verify_log_message -2 "*Pausing writes (remaining_repl_size is 0) to allow slot migration*" 0 + } +} + +start_cluster 3 0 {tags {logreqres:skip external:skip cluster} overrides {cluster-require-full-coverage no slot-migration-max-failover-repl-bytes -1}} { + test "slot-migration-max-failover-repl-bytes -1 disables repl bytes limit" { + set 16383_slot_tag "{6ZJ}" + + # Use PREVENT-PAUSE to hold the migration at the waiting-to-pause + # state so we can fill the replication buffer first. + R 2 DEBUG SLOTMIGRATION PREVENT-PAUSE 1 + + # Move slot 16383 from R2 to R0. + assert_match "OK" [R 2 CLUSTER MIGRATESLOTS SLOTSRANGE 16383 16383 NODE [R 0 CLUSTER MYID]] + set jobname [get_job_name 2 16383] + wait_for_migration_field 2 $jobname state waiting-to-pause + + # Pause the target (R0) process and generate data to fill the + # replication buffer on the source (R2). + pause_process [srv 0 pid] + set bigstr [string repeat x 1024000] + for {set j 0} {$j < 50} {incr j} { + R 2 set "$16383_slot_tag:key:$j" $bigstr + } + + # Confirm the replication buffer has accumulated data. + set export_migration [get_migration_by_name 2 $jobname] + set remaining_repl_size [dict get $export_migration remaining_repl_size] + assert_morethan $remaining_repl_size [expr 1024000 * 25] + + # Resume the PREVENT-PAUSE. + # With slot-migration-max-failover-repl-bytes -1, the source (R2) should proceed + # to pause writes regardless of the large remaining_repl_size. + R 2 DEBUG SLOTMIGRATION PREVENT-PAUSE 0 + wait_for_log_messages -2 {"*Pausing writes*"} 0 1000 10 + set pattern "*Pausing writes (remaining_repl_size is $remaining_repl_size) to allow slot migration*" + verify_log_message -2 $pattern 0 + + # Resume R0 and wait for R0 to finish the migration. + resume_process [srv 0 pid] + wait_for_migration 0 16383 } } diff --git a/tests/unit/introspection.tcl b/tests/unit/introspection.tcl index c5c456b5134..8fd5006a91c 100644 --- a/tests/unit/introspection.tcl +++ b/tests/unit/introspection.tcl @@ -1964,10 +1964,22 @@ test {CONFIG REWRITE handles alias config properly} { } } {} {external:skip} +test {SIGNED MEMORY CONFIG allows negative number} { + start_server {tags {"introspection"}} { + r config set slot-migration-max-failover-repl-bytes -1 + assert_equal [lindex [r config get slot-migration-max-failover-repl-bytes] 1] -1 + assert_error {*argument must be between -1 and *} {r config set slot-migration-max-failover-repl-bytes -2} + + r config rewrite + restart_server 0 true false + assert_equal [lindex [r config get slot-migration-max-failover-repl-bytes] 1] -1 + } +} {} {external:skip} + test {CONFIG hash-seed is immutable and settable at startup} { start_server {tags {"introspection"} overrides {hash-seed aabbccddeeffgghh}} { assert_error "ERR CONFIG SET failed (possibly related to argument 'hash-seed') - can't set immutable config*" { r config set hash-seed newseed } } -} {} {external:skip} \ No newline at end of file +} {} {external:skip}