In #9389, we add a new `cluster-port` config and make cluster bus port configurable,
and currently redis-cli --cluster create/add-node doesn't support with a configurable `cluster-port` instance.
Because redis-cli uses the old way (port + 10000) to send the `CLUSTER MEET` command.
Now we add this support on redis-cli `--cluster`, note we don't need to explicitly pass in the
`cluster-port` parameter, we can get the real `cluster-port` of the node in `clusterManagerNodeLoadInfo`,
so the `--cluster create` and `--cluster add-node` interfaces have not changed.
We will use the `cluster-port` when we are doing `CLUSTER MEET`, also note that `CLUSTER MEET` bus-port
parameter was added in 4.0, so if the bus_port (the one in redis-cli) is 0, or equal (port + 10000),
we just call `CLUSTER MEET` with 2 arguments, using the old form.
Co-authored-by: Madelyn Olson <34459052+madolson@users.noreply.github.com>
CONTRIBUTING to get better formatting
CONDUCT also because github doesn't seem recognize the code of conduct page
Co-authored-by: Oran Agra <oran@redislabs.com>
Use SSL_shutdown(), in a best-effort manner, when closing a TLS
connection. This change better supports OpenSSL 3.x clients that will
not silently ignore the socket-level EOF.
## Issue
During the MULTI/EXEC flow, each command gets queued until the `EXEC`
command is received and during this phase on every command queue, a
`realloc` is being invoked. This could be expensive based on the realloc
behavior (if copy to a new memory location).
## Solution
In order to reduce the no. of syscall, couple of optimization I've used.
1. By default, reserve memory for atleast two commands. `MULTI/EXEC` for a
single command doesn't have any significance. Hence, I believe customer wouldn't use it.
2. For further reservation, increase the memory allocation in exponent growth (power of 2).
This reduces the no. of `realloc` call from `N` to `log(N)` times.
## Other changes:
* Include multi exec queued command array in client memory consumption calculation
(affects client eviction too)
Currently in cluster mode, Redis process locks the cluster config file when
starting up and holds the lock for the entire lifetime of the process.
When the server shuts down, it doesn't explicitly release the lock on the
cluster config file. We noticed a problem with restart testing that if you shut down
a very large redis-server process (i.e. with several hundred GB of data stored),
it takes the OS a while to free the resources and unlock the cluster config file.
So if we immediately try to restart the redis server process, it might fail to acquire
the lock on the cluster config file and fail to come up.
This fix explicitly releases the lock on the cluster config file upon a shutdown rather
than relying on the OS to release the lock, which is a cleaner and safer approach to
free up resources acquired.
Account sharded pubsub channels memory consumption in client memory usage
computation to accurately evict client based on the set threshold for `maxmemory-clients`.
We should also set aof_lastbgrewrite_status to C_ERR on these
errors. Because aof rewrite did fail, and we did not finish the
manifest update. Also maintain the stat_aofrw_consecutive_failures.
The `can_log` variable prevents us from outputting too
many error logs. But it should not include the modification
of server.aof_last_write_errno.
We are doing this because:
1. In the short write case, we always set aof_last_write_errno
to ENOSPC, we don't care the `can_log` flag.
2. And we always set aof_last_write_status to C_ERR in aof write
error (except for FSYNC_ALWAYS, we exit). So there may be a chance
that `aof_last_write_errno` is not right.
An innocent bug or just a code cleanup.
This was harmless because we marked the parent command
with SENTINEL flag. So the populateCommandTable was ok.
And we also don't show the flag (SENTINEL and ONLY-SENTNEL)
in COMMAND INFO.
In this PR, we also add the same CMD_SENTINEL and CMD_ONLY_SENTINEL
flags check when populating the sub-commands.
so that in the future it'll be possible to add some sub-commands to sentinel or sentinel-only but not others.
Fix regression of CLUSTER RESET command in redis 7.0.
cluster reset command format is:
CLUSTER RESET [ HARD | SOFT]
According to the cluster reset command doc and codes, the third argument is optional, so
the arity in json file should be -2 instead of 3.
Add test to verify future regressions with RESET and RESET SOFT that were not covered.
Co-authored-by: Ubuntu <lucas.guang.yang1@huawei.com>
Co-authored-by: Oran Agra <oran@redislabs.com>
Co-authored-by: Binbin <binloveplay1314@qq.com>
When calling CLIENT INFO/LIST, and in various debug prints, Redis is printing
the number of pubsub channels / patterns the client is subscribed to.
With the addition of sharded pubsub, it would be useful to print the number of
keychannels the client is subscribed to as well.
The module API docs mentions this macro, but it was not defined (so no one could have used it).
Instead of adding it as is, we decided to add a _V1 macro, so that if / when we some day extend this struct,
modules that use this API and don't need the extra fields, will still use the old version
and still be compatible with older redis version (despite being compiled with newer redismodule.h)
Since the ranges of `unsigned long long` and `long long` are different, we cannot read an
`unsigned long long` integer from a `RedisModuleString` by `RedisModule_StringToLongLong` .
So I added two new Redis Module APIs to support the conversion between these two types:
* `RedisModule_StringToULongLong`
* `RedisModule_CreateStringFromULongLong`
Signed-off-by: RinChanNOWWW <hzy427@gmail.com>
This commit has two topics.
## Passing config name and value in the same arg
In #10660 (Redis 7.0.1), when we supported the config values that can start with `--` prefix (one of the two topics of that PR),
we broke another pattern: `redis-server redis.config "name value"`, passing both config name
and it's value in the same arg, see #10865
This wasn't a intended change (i.e we didn't realize this pattern used to work).
Although this is a wrong usage, we still like to fix it.
Now we support something like:
```
src/redis-server redis.conf "--maxmemory '700mb'" "--maxmemory-policy volatile-lru" --proc-title-template --my--title--template --loglevel verbose
```
## Changes around --save
Also in this PR, we undo the breaking change we made in #10660 on purpose.
1. `redis-server redis.conf --save --loglevel verbose` (missing `save` argument before anotehr argument).
In 7.0.1, it was throwing an wrong arg error.
Now it will work and reset the save, similar to how it used to be in 7.0.0 and 6.2.x.
3. `redis-server redis.conf --loglevel verbose --save` (missing `save` argument as last argument).
In 6.2, it did not reset the save, which was a bug (inconsistent with the previous bullet).
Now we will make it work and reset the save as well (a bug fix).
This is harmless, we only restore mstate to make sure we
free the right pointer in freeClientMultiState, but it'll
be nicer to also sync that argv_len var back.
There are a lot of false sharing cache misses in line 4013 inside getIOPendingCount function.
The reason is that elements of io_threads_pending array access the same cache line from different
threads, so it is better to represent it as an array of structures with fields aligned to cache line size.
This change should improve performance (in particular, it affects the latency metric, we saw up to 3% improvement).
In 6.2.0 with the introduction of the REV subcommand in ZRANGE, there was a semantic shift in the arguments of ZRANGE when the REV sub-command is executed. Without the sub-command `min` and `max` (the old names of the arguments) are appropriate because if you put the min value and the max value in everything works fine.
```bash
127.0.0.1:6379> ZADD myset 0 foo
(integer) 1
127.0.0.1:6379> ZADD myset 1 bar
(integer) 1
127.0.0.1:6379> ZRANGE myset 0 inf BYSCORE
1) "foo"
2) "bar"
```
However - if you add the `REV` subcommand, ordering the arguments `min` `max` breaks the command:
```bash
127.0.0.1:6379> ZRANGE myset 0 inf BYSCORE REV
(empty array)
```
why? because `ZRANGE` with the `REV` sub-command is expecting the `max` first and the `min` second (because it's a reverse range like `ZREVRANGEBYSCORE`):
```bash
127.0.0.1:6379> ZRANGE myset 0 inf BYSCORE REV
(empty array)
```
The new test added in #10891 can fail with a different error.
see comment in networking.c saying
```c
/* That's a best effort error message, don't check write errors.
* Note that for TLS connections, no handshake was done yet so nothing
* is written and the connection will just drop. */
```
In `CLUSTER MEET`, bus-port argument was added in 11436b1.
For cluster announce ip / port implementation, part of the
4.0-RC1.
And in #9389, we add a new cluster-port config and make
cluster bus port configurable, part of the 7.0-RC1.
my maxclients config:
```
redis-cli config get maxclients
1) "maxclients"
2) "4064"
```
Before this bug was fixed, creating 4065 clients appeared to be successful, but only 4064 were actually created```
```
./redis-benchmark -c 4065 -I
Creating 4065 idle connections and waiting forever (Ctrl+C when done)
cients: 4065
```
now :
```
./redis-benchmark -c 4065 -I
Creating 4065 idle connections and waiting forever (Ctrl+C when done)
Error from server: ERR max number of clients reached
./redis-benchmark -c 4064 -I
Creating 4064 idle connections and waiting forever (Ctrl+C when done)
clients: 4064
```
* Fix typo `RedisModule_CreatString` -> `RedisModule_CreateString` (multiple occurrences)
* Make the markdown gen script change all `RM_` to `RedisModule_` even in code examples, etc.
The PR fixes 2 issues:
### RM_Call crash on script mode
`RM_Call` can potentially be called from a background thread where `server.current_client`
are not set. In such case we get a crash on `NULL` dereference.
The fix is to check first if `server.current_client` is `NULL`, if it does we should
verify disc errors and readonly replica as we do to any normal clients (no masters nor AOF).
### RM_Call block OOM commands when not needed
Again `RM_Call` can be executed on a background thread using a `ThreadSafeCtx`.
In such case `server.pre_command_oom_state` can be irrelevant and should not be
considered when check OOM state. This cause OOM commands to be blocked when
not necessarily needed.
In such case, check the actual used memory (and not the cached value). Notice that in
order to know if the cached value can be used, we check that the ctx that was used on
the `RM_Call` is a ThreadSafeCtx. Module writer can potentially abuse the API and use
ThreadSafeCtx on the main thread. We consider this as a API miss used.
The current process to persist files is `write` the data, `fsync` and `rename` the file,
but a underlying problem is that the rename may be lost when a sudden crash like
power outage and the directory hasn't been persisted.
The article [Ensuring data reaches disk](https://lwn.net/Articles/457667/) mentions
a safe way to update file should be:
1. create a new temp file (on the same file system!)
2. write data to the temp file
3. fsync() the temp file
4. rename the temp file to the appropriate name
5. fsync() the containing directory
This commit handles CONFIG REWRITE, AOF manifest, and RDB file (both for persistence,
and the one the replica gets from the master).
It doesn't handle (yet), ACL SAVE and Cluster configs, since these don't yet follow this pattern.
Clients could use this config in sentinel only when they already add rename-command in the redis conf file,
but becuase in Redis instance, we already DEPRECATED rename-command config,
thus we should deprecate this in the sentinel part as well.
Co-authored-by: Oran Agra <oran@redislabs.com>
when we know the size of the zset we're gonna store in advance,
we can check if it's greater than the listpack encoding threshold,
in which case we can create a skiplist from the get go, and avoid
converting the listpack to skiplist later after it was already populated.
If a script made a modification and then was interrupted for taking too long.
there's a chance redis will detect that a replica dropped and would like to reject
write commands with NOREPLICAS due to insufficient good replicas.
returning an error on a command in this case breaks the script atomicity.
The same could in theory happen with READONLY, MISCONF, but i don't think
these state changes can happen during script execution.
I noticed that scripting.tcl uses INFO from within a script and thought it's an
overkill and concluded it's nicer to use another CMD_STALE command,
decided to use ECHO, and then noticed it's not at all allowed in stale mode.
probably overlooked at #6843
* In cluster getNodeByQuery when target slot is in migrating state and
the slot lack some keys but have at least one key, should return TRYAGAIN.
Before this commit, when a node is in migrating state and recevies
multiple keys command, if some keys don't exist, the command emits
an `ASK` redirection.
After this commit, if some keys exist and some keys don't exist, the
command emits a TRYAGAIN error. If all keys don't exist, the command
emits an `ASK` redirection.
