- Supports `VALKEYMODULE_CONFIG_UNSIGNED` and `UNSIGNED_CONFIG` for a
wider range of configurations.
- Supported the API for `ValkeyModule_RegisterUnsignedNumericConfig`
- `string2ull` method supports the length parameter, making it more
secure
---------
Signed-off-by: artikell <[email protected]>
Signed-off-by: skyfirelee <[email protected]>
Signed-off-by: Madelyn Olson <[email protected]>
Co-authored-by: Madelyn Olson <[email protected]>
Add VM_ClusterKeySlotC that takes a data pointer and len as parameters.
Purpose: A module can avoid creating a ValkeyModuleString in a hot code
path.
Closes#2901
---------
Signed-off-by: Su Ko <[email protected]>
Signed-off-by: Viktor Söderqvist <[email protected]>
Co-authored-by: Viktor Söderqvist <[email protected]>
## API changes and user behavior:
- [x] Default behavior for database access.
Default is `alldbs` permissions.
### Database Permissions (`db=`)
- [x] Accessing particular database
```
> ACL SETUSER test1 on +@all ~* resetdbs db=0,1 nopass
"user test1 on nopass sanitize-payload ~* resetchannels db=0,1 +@all"
```
- [x] (Same behavior without usage of `resetdbs`)
```
> ACL SETUSER test1 on +@all ~* db=0,1 nopass
"user test1 on nopass sanitize-payload ~* resetchannels db=0,1 +@all"
```
- [x] Multiple selector can be provided
```
> ACL SETUSER test1 on nopass (db=0,1 +@write +select ~*) (db=2,3 +@read +select ~*)
"user test1 on nopass sanitize-payload resetchannels alldbs -@all (~* resetchannels db=0,1 -@all +@write +select) (~* resetchannels db=2,3 -@all +@read +select)"
```
- [x] Restricting special commands which access databases as part of the
command.
The user needs to have access to both the commands and db(s) part of the
command to run these commands.
1. SWAPDB
2. SELECT
3. MOVE - (Select command would have went through for the source
database). Have access for the target database.
4. COPY
- [x] Restricting special commands which doesn't specify database
number, however, accesses multiple databases.
The user needs to have access to both the commands and all databases
(`alldbs`) to run these commands.
1. FLUSHALL - Access all databases
2. CLUSTER commands that access all databases:
- CANCELSLOTMIGRATIONS
- MIGRATESLOTS
- [x] New connection establishment behavior
New client connection gets established to DB 0 by default.
Authentication and authorisation are decoupled and the user can
connect/authenticate and further perform `SELECT` or other operation
that do not access keyspace.
(Do we want to extend HELLO?) Alternative suggestion by @madolson:
Extend `HELLO` command to pass the dbid to which the user should get
connected after authentication if they have right set of permission. I
think it will become a long poll for adoption.
- [x] Observability
Extend `ACL LOG` to log user which received denied permission error
while accessing a database.
- [x] Module API
* Introduce module API `int VM_ACLCheckPermissions(ValkeyModuleUser
*user, ValkeyModuleString **argv, int argc, int dbid,
ValkeyModuleACLLogEntryReason *denial_reason);`
* Stop support of `VM_ACLCheckCommandPermissions()`.
Resolves: #1336
---------
Signed-off-by: Daniil Kashapov <[email protected]>
## Overview
Sharing memory between the module and engine reduces memory overhead by
eliminating redundant copies of stored records in the module. This is
particularly beneficial for search workloads that require indexing large
volumes of documents.
### Vectors
Vector similarity search requires storing large volumes of
high-cardinality vectors. For example, a single vector with 512
dimensions consumes 2048 bytes, and typical workloads often involve
millions of vectors. Due to the lack of a memory-sharing mechanism
between the module and the engine, valkey-search currently doubles
memory consumption when indexing vectors, significantly increasing
operational costs. This limitation introduces adoption friction and
reduces valkey-search's competitiveness.
## Memory Allocation Strategy
At a fundamental level, there are two primary allocation strategies:
- [Chosen] Module-allocated memory shared with the engine.
- Engine-allocated memory shared with the module.
For valkey-search, it is crucial that vectors reside in cache-aligned
memory to maximize SIMD optimizations. Allowing the module to allocate
memory provides greater flexibility for different use cases, though it
introduces slightly higher implementation complexity.
## Old Implementation
The old [implementation](https://github.com/valkey-io/valkey/pull/1804)
was based on ref-counting and introduced a new SDS type. After further
discussion, we
[agreed](https://github.com/valkey-io/valkey/pull/1804#issuecomment-2905115712)
to simplify the design by removing ref-counting and avoiding the
introduction of a new SDS type.
## New Implementation - Key Points
1. The engine exposes a new interface, `VM_HashSetViewValue`, which set
value as a view of a buffer which is owned by the module. The function
accepts the hash key, hash field, and a buffer along with its length.
2. `ViewValue` is a new data type that captures the externalized buffer
and its length.
## valkey-search Usage
### Insertion
1. Upon receiving a key space notification for a new hash or JSON key
with an indexed vector attribute, valkey-search allocates cache-aligned
memory and deep-copies the vector value.
2. valkey-search then calls `VM_HashSetViewValue` to avoid keeping two
copies of the vector.
### Deletion
When receiving a key space notification for a deleted hash key or hash
field that was indexed as a vector, valkey-search deletes the
corresponding entry from the index.
### Update
Handled similarly to insertion.
---------
Signed-off-by: yairgott <[email protected]>
Signed-off-by: Yair Gottdenker <[email protected]>
Signed-off-by: Yair Gottdenker <[email protected]>
Co-authored-by: Yair Gottdenker <[email protected]>
Co-authored-by: Ran Shidlansik <[email protected]>
Co-authored-by: Jim Brunner <[email protected]>
This PR restructures the Lua scripting functionality by extracting
it from the core Valkey server into a separate Valkey module. This
change enables the possibility of a backwards compatible Lua engine
upgrade, as well as, the flexibility in building Valkey without the
Lua engine.
**Important**: from a user's point of view, there's no difference in
using the `EVAL` of `FUNCTION/FCALL` scripts. This PR is fully backward
compatible with respect to the public API.
The main code change is the move and adaptation of the Lua engine source
files from `src/lua` to `src/modules/lua`. The original Lua engine code is
adapted to use the module API to compile and execute scripts.
The main difference between the original code and the new, is the
serialization and deserialization of Valkey RESP values into, and from,
Lua values. While in the original implementation the parsing of RESP
values
was done directly from the client buffer, in the new implementation the
parsing is done from the `ValkeyModuleCallReply` object and respective
API.
The Makefile and CMake build systems were also updated to build and
integrate the new Lua engine module, within the Valkey server build
workflow.
When the Valkey server is built, the Lua engine module is also built,
and, the Lua module is loaded automatically by the server upon startup.
When running `make install` the Lua engine module is installed in the
default system library directory.
There's a new build option, called `BUILD_LUA`, that if set to `no`
allows to
build Valkey server without building the Lua engine.
This modular architecture enables future development of additional Lua
engine modules with newer Lua versions that can be loaded alongside the
current engine, facilitating gradual migration paths for users.
Additional change: Unload all modules on shutdown (ignoring modules that
can't be unloaded). This is to avoid address sanitizer warnings about
leaked allocations.
Fixes: #1627
---------
Signed-off-by: Ricardo Dias <[email protected]>
The CLUSTER SLOTS reply depends on whether the client is connected over
IPv6, but for a fake client there is no connection and when this command
is called from a module timer callback or other scenario where no real
client is involved, there is no connection to check IPv6 support on.
This fix handles the missing case by returning the reply for IPv4
connected clients.
Fixes#2912.
---------
Signed-off-by: Su Ko <[email protected]>
Signed-off-by: Binbin <[email protected]>
Signed-off-by: Viktor Söderqvist <[email protected]>
Co-authored-by: Su Ko <[email protected]>
Co-authored-by: KarthikSubbarao <[email protected]>
Co-authored-by: Binbin <[email protected]>
Co-authored-by: Viktor Söderqvist <[email protected]>
This commit adds script function flags to the module API, which allows
function scripts to specify the function flags programmatically.
When the scripting engine compiles the script code can extract the flags
from the code and set the flags on the compiled function objects.
---------
Signed-off-by: Ricardo Dias <[email protected]>
By default, when the number of elements in a zset exceeds 128, the
underlying data structure adopts a skiplist. We can reduce memory usage
by embedding elements into the skiplist nodes. Change the `zskiplistNode`
memory layout as follows:
```
Before
+-------------+
+-----> | element-sds |
| +-------------+
|
+------------------+-------+------------------+---------+-----+---------+
| element--pointer | score | backward-pointer | level-0 | ... | level-N |
+------------------+-------+------------------+---------+-----+---------+
After
+-------+------------------+---------+-----+---------+-------------+
+ score | backward-pointer | level-0 | ... | level-N | element-sds |
+-------+------------------+---------+-----+---------+-------------+
```
Before the embedded SDS representation, we include one byte representing
the size of the SDS header, i.e. the offset into the SDS representation
where that actual string starts.
The memory saving is therefore one pointer minus one byte = 7 bytes per
element, regardless of other factors such as element size or number of
elements.
### Benchmark step
I generated the test data using the following lua script && cli command.
And check memory usage using the `info` command.
**lua script**
```
local start_idx = tonumber(ARGV[1])
local end_idx = tonumber(ARGV[2])
local elem_count = tonumber(ARGV[3])
for i = start_idx, end_idx do
local key = "zset:" .. string.format("%012d", i)
local members = {}
for j = 0, elem_count - 1 do
table.insert(members, j)
table.insert(members, "member:" .. j)
end
redis.call("ZADD", key, unpack(members))
end
return "OK: Created " .. (end_idx - start_idx + 1) .. " zsets"
```
**valkey-cli command**
`valkey-cli EVAL "$(catcreate_zsets.lua)" 0 0 100000
${ZSET_ELEMENT_NUM}`
### Benchmark result
|number of elements in a zset | memory usage before optimization |
memory usage after optimization | change |
|-------|-------|-------|-------|
| 129 | 1047MB | 943MB | -9.9% |
| 256 | 2010MB| 1803MB| -10.3%|
| 512 | 3904MB|3483MB| -10.8%|
---------
Signed-off-by: chzhoo <[email protected]>
Co-authored-by: Viktor Söderqvist <[email protected]>
## Description
This change introduces the ability for modules to check ACL permissions
against key prefix. The update adds a dedicated `prefixmatchlen` helper
and extends the core ACL selector logic to support a prefix‑matching
mode.
The new API `ValkeyModule_ACLCheckPrefixPermissions` is registered and
exposed to modules, and a corresponding implementation is added in
`module.c`. Existing internal callers that already perform prefix checks
(e.g., `VM_ACLCheckKeyPermissions`) are updated to use the new flag,
while all legacy paths remain unchanged.
The change also modifies the `aclcheck§ test module that exercises the
new prefix‑checking API, ensuring that read/write operations are
correctly allowed or denied based on the ACL configuration.
Key areas touched:
* ACL logic
* Module API
* Testing
# Motivation
The search module presently makes costly calls to verify index
permissions
(see https://github.com/valkey-io/valkey-search/blob/main/src/acl.cc#L295).
This PR introduces a more efficient approach for that.
---------
Signed-off-by: Eran Ifrah <[email protected]>
Signed-off-by: Madelyn Olson <[email protected]>
Signed-off-by: Ran Shidlansik <[email protected]>
Co-authored-by: Madelyn Olson <[email protected]>
Co-authored-by: Ran Shidlansik <[email protected]>
Co-authored-by: Binbin <[email protected]>
This commit extends the Module API to expose the `SIMPLE_STRING` and
`ARRAY_NULL` reply types to modules, by passing the new flag `X` to
the `ValkeyModule_Call` function.
By only exposing the new reply types behind a flag we keep the
backward compatibility with existing module implementations and
allow new modules to working with these reply type, which are
required for scripts to process correctly the reply type of commands
called inside scripts.
Before this change, commands like `PING` or `SET`, which return `"OK"`
as a simple string reply, would be returned as string replies to
scripts.
To allow the support of the Lua engine as an external module, we need to
distinguish between simple string and string replies to keep backward
compatibility.
---------
Signed-off-by: Ricardo Dias <[email protected]>
Note: these changes are part of the effort to run Lua engine as an
external scripting engine module.
The new function `ValkeyModule_ReplyWithCustomErrorFormat` is being
added to the module API to allow scripting engines to return errors that
originated from running commands within the script code, without
counting twice in the error stats counters.
More details on why this is needed by scripting engines can be read in
an older commit aa856b39f2 messsage.
This PR also adds a new test to ensure the correctness of the newly
added function.
---------
Signed-off-by: Ricardo Dias <[email protected]>
Note: these changes are another step towards being able to run Lua
engine as an external scripting engine module.
In this commit we improve the `ValkeyModule_Call` API function code to
match the validations and behavior of the `scriptCall` function,
currently used by the Lua engine to run commands using `server.call` Lua
Valkey API.
The changes made are backward compatible. The new behavior/validations
are only enabled when calling `ValkeyModule_Call` while running a script
using `EVAL` or `FCALL`.
To test these changes, we improved the `HELLO` dummy scripting engine
module to support calling commands, and compare the behavior with
calling the same command from a Lua script.
Signed-off-by: Ricardo Dias <[email protected]>
This PR introduces the support for implementing remote debuggers in
scripting engines modules.
The module API is extended with scripting engines callbacks and new
functions that can be used by scripting engine modules to implement a
remote debugger.
Most of the code that was used to implement the Lua debugger, was
refactored and moved to the `scripting_engine.c` file, and only the code
specific to the Lua engine, remained in the `debug_lua.c` file.
The `SCRIPT DEBUG (YES|NO|SYNC)` command was extend with an optional
parameter that can be used to specify the engine name, where we want to
enable the debugger. If no engine name is specified, the Lua engine is
used to keep backwards compatibility.
In
[src/valkeymodule.h](https://github.com/valkey-io/valkey/pull/1701/files#diff-b91520205c29a3a5a940786e509b2f13a5e73a1ac2016be773e62ea64c7efb28)
we see the module API changes. And in the `helloscripting.c` file we can
see how to implement a simple debugger for the dummy HELLO scripting
engine.
---------
Signed-off-by: Ricardo Dias <[email protected]>
There will be two issues in this test:
```
test {FUNCTION - test function flush} {
for {set i 0} {$i < 10000} {incr i} {
r function load [get_function_code LUA test_$i test_$i {return 'hello'}]
}
set before_flush_memory [s used_memory_vm_functions]
r function flush sync
set after_flush_memory [s used_memory_vm_functions]
puts "flush sync, before_flush_memory: $before_flush_memory, after_flush_memory: $after_flush_memory"
for {set i 0} {$i < 10000} {incr i} {
r function load [get_function_code LUA test_$i test_$i {return 'hello'}]
}
set before_flush_memory [s used_memory_vm_functions]
r function flush async
set after_flush_memory [s used_memory_vm_functions]
puts "flush async, before_flush_memory: $before_flush_memory, after_flush_memory: $after_flush_memory"
for {set i 0} {$i < 10000} {incr i} {
r function load [get_function_code LUA test_$i test_$i {return 'hello'}]
}
puts "Test done"
}
```
The first one is the test output, we can see that after executing
FUNCTION FLUSH,
used_memory_vm_functions has not changed at all:
```
flush sync, before_flush_memory: 2962432, after_flush_memory: 2962432
flush async, before_flush_memory: 4504576, after_flush_memory: 4504576
```
The second one is there is a crash when loading the functions during the
async
flush:
```
=== VALKEY BUG REPORT START: Cut & paste starting from here ===
# valkey 255.255.255 crashed by signal: 11, si_code: 2
# Accessing address: 0xe0429b7100000a3c
# Crashed running the instruction at: 0x102e0b09c
------ STACK TRACE ------
EIP:
0 valkey-server 0x0000000102e0b09c luaH_getstr + 52
Backtrace:
0 libsystem_platform.dylib 0x000000018b066584 _sigtramp + 56
1 valkey-server 0x0000000102e01054 luaD_precall + 96
2 valkey-server 0x0000000102e01b10 luaD_call + 104
3 valkey-server 0x0000000102e00d1c luaD_rawrunprotected + 76
4 valkey-server 0x0000000102e01e3c luaD_pcall + 60
5 valkey-server 0x0000000102dfc630 lua_pcall + 300
6 valkey-server 0x0000000102f77770 luaEngineCompileCode + 708
7 valkey-server 0x0000000102f71f50 scriptingEngineCallCompileCode + 104
8 valkey-server 0x0000000102f700b0 functionsCreateWithLibraryCtx + 2088
9 valkey-server 0x0000000102f70898 functionLoadCommand + 312
10 valkey-server 0x0000000102e3978c call + 416
11 valkey-server 0x0000000102e3b5b8 processCommand + 3340
12 valkey-server 0x0000000102e563cc processInputBuffer + 520
13 valkey-server 0x0000000102e55808 readQueryFromClient + 92
14 valkey-server 0x0000000102f696e0 connSocketEventHandler + 180
15 valkey-server 0x0000000102e20480 aeProcessEvents + 372
16 valkey-server 0x0000000102e4aad0 main + 26412
17 dyld 0x000000018acab154 start + 2476
------ STACK TRACE DONE ------
```
The reason is that, in the old implementation (introduced in 7.0),
FUNCTION FLUSH
use lua_unref to remove the script from lua VM. lua_unref does not
trigger the gc,
it causes us to not be able to effectively reclaim memory after the
FUNCTION FLUSH.
The other issue is that, since we don't re-create the lua VM in FUNCTION
FLUSH,
loading the functions during a FUNCTION FLUSH ASYNC will result a crash
because
lua engine state is not thread-safe.
The correct solution is to re-create a new Lua VM to use, just like
SCRIPT FLUSH.
---------
Signed-off-by: Binbin <[email protected]>
Signed-off-by: Ricardo Dias <[email protected]>
Co-authored-by: Ricardo Dias <[email protected]>
# Problem
In the current slot migration design, replicas are completely unaware of
the slot migration. Because of this, they do not know to hide importing
keys, which results in exposure of these keys to commands like KEYS,
SCAN, RANDOMKEY, and DBSIZE.
# Design
The main part of the design is that we will now listen for and process
the `SYNCSLOTS ESTABLISH` command on the replica. When a `SYNCSLOTS
ESTABLISH` command is received from the primary, we begin tracking a new
slot import in a special `SLOT_IMPORT_OCCURRING_ON_PRIMARY` state.
Replicas use this state to track the import, and await for a future
`SYNCSLOTS FINISH` message that tells them the import is
successful/failed.
## Success Case
```
Source Target Target Replica
| | |
|------------ SYNCSLOTS ESTABLISH -------------->| |
| |----- SYNCSLOTS ESTABLISH ------>|
|<-------------------- +OK ----------------------| |
| | |
|~~~~~~~~~~~~~~ snapshot as AOF ~~~~~~~~~~~~~~~~>| |
| |~~~~~~ forward snapshot ~~~~~~~~>|
|----------- SYNCSLOTS SNAPSHOT-EOF ------------>| |
| | |
|<----------- SYNCSLOTS REQUEST-PAUSE -----------| |
| | |
|~~~~~~~~~~~~ incremental changes ~~~~~~~~~~~~~~>| |
| |~~~~~~ forward changes ~~~~~~~~~>|
|--------------- SYNCSLOTS PAUSED -------------->| |
| | |
|<---------- SYNCSLOTS REQUEST-FAILOVER ---------| |
| | |
|---------- SYNCSLOTS FAILOVER-GRANTED --------->| |
| | |
| (performs takeover & |
| propagates topology) |
| | |
| |------- SYNCSLOTS FINISH ------->|
(finds out about topology | |
change & marks migration done) | |
| | |
```
## Failure Case
```
Source Target Target Replica
| | |
|------------ SYNCSLOTS ESTABLISH -------------->| |
| |----- SYNCSLOTS ESTABLISH ------>|
|<-------------------- +OK ----------------------| |
... ... ...
| | |
| <FAILURE> |
| | |
| (performs cleanup) |
| | ~~~~~~ UNLINK <key> ... ~~~~~~~>|
| | |
| | ------ SYNCSLOTS FINISH ------->|
| | |
```
## Full Sync, Partial Sync, and RDB
In order to ensure replicas that resync during the import are still
aware of the import, the slot import is serialized to a new
`cluster-slot-imports` aux field. The encoding includes the job name,
the source node name, and the slot ranges being imported. Upon loading
an RDB with the `cluster-slot-imports` aux field, replicas will add a
new migration in the `SLOT_IMPORT_OCCURRING_ON_PRIMARY` state.
It's important to note that a previously saved RDB file can be used as
the basis for partial sync with a primary. Because of this, whenever we
load an RDB file with the `cluster-slot-imports` aux field, even from
disk, we will still add a new migration to track the import. If after
loading the RDB, the Valkey node is a primary, it will cancel the slot
migration. Having this tracking state loaded on primaries will ensure
that replicas partial syncing to a restarted primary still get their
`SYNCSLOTS FINISH` message in the replication stream.
## AOF
Since AOF cannot be used as the basis for a partial sync, we don't
necessarily need to persist the `SYNCSLOTS ESTABLISH` and `FINISH`
commands to the AOF.
However, considering there is work to change this (#59#1901) this
design doesn't make any assumptions about this.
We will propagate the `ESTABLISH` and `FINISH` commands to the AOF, and
ensure that they can be properly replayed on AOF load to get to the
right state. Similar to RDB, if there are any pending "ESTABLISH"
commands that don't have a "FINISH" afterwards upon becoming primary, we
will make sure to fail those in `verifyClusterConfigWithData`.
Additionally, there was a bug in the existing slot migration where slot
import clients were not having their commands persisted to AOF. This has
been fixed by ensuring we still propagate to AOF even for slot import
clients.
## Promotion & Demotion
Since the primary is solely responsible for cleaning up unowned slots,
primaries that are demoted will not clean up previously active slot
imports. The promoted replica will be responsible for both cleaning up
the slot (`verifyClusterConifgWithData`) and sending a `SYNCSLOTS
FINISH`.
# Other Options Considered
I also considered tracking "dirty" slots rather than using the slot
import state machine. In this setup, primaries and replicas would simply
mark each slot's hashtable in the kvstore as dirty when something is
written to it and we do not currently own that slot.
This approach is simpler, but has a problem in that modules loaded on
the replica would still not get slot migration start/end notifications.
If the modules on the replica do not get such notifications, they will
not be able to properly contain these dirty keys during slot migration
events.
---------
Signed-off-by: Jacob Murphy <[email protected]>
As discussed in https://github.com/valkey-io/valkey/issues/2579
Notably, I am exposing this feature as "Atomic Slot Migration" to
modules. If we want to call it something else, we should consider that
now (e.g.. "Replication-Based Slot Migration"?)
Also note: I am exposing both target and source node in the event. It is
not guaranteed that either target or source would be the node the event
fires on (e.g. replicas will fire the event after replica containment is
introduced). Even though it could be potentially inferred from CLUSTER
SLOTS, it should help modules parse it this way. Modules should be able
to infer whether it is occurring on primary/replica from `ctx` flags, so
not duplicating that here.
Closes#2579
---------
Signed-off-by: Jacob Murphy <[email protected]>
This flag.deny_blocking check causes some code to becomde dead code.
Some of the code below checks islua or ismulti, but they are marked
with flag.deny_blocking and will return early.
In addition, cleanup some documents, some of them are inaccurate,
and restore the code of blocking_auth_cb in tests/modules/auth.c,
this reply should be returned from core. The reply used to from the
core and was changed to from the module in #1819, and now it is from
the core again.
Cleanup some dead code around #1819.
Signed-off-by: Binbin <[email protected]>
In this commit we introduce a new module API event called
`ValkeyModuleEvent_AuthenticationAttempt` to track successful/failed
authentication attempts.
This event will fill a struct, called `ValkeyModuleAuthenticationInfo`,
with the client ID of the connection, the username, the module name that
handle the authentication event, and the result of the authentication
attempt.
Fixes: #2211
---------
Signed-off-by: Ricardo Dias <[email protected]>
* Add a flag `VALKEYMODULE_CLIENTINFO_FLAG_READONLY` to
`ValkeyModuleClientInfo.flags` and set it in
`ValkeyModule_GetClientInfoById()`.
* Add an optimization for accessing the current client by ID, to avoid
looking it up in a radix tree.
Closes#2487
---------
Signed-off-by: Allen Samuels <[email protected]>
See #2132 for the fix.
This just ensures that we always create context in all branches of
aux_load and aux_save
Signed-off-by: Jacob Murphy <[email protected]>
We made a change during the refactoring of the engine changes that used
a strlen on an SDS string. SDS are resizable, but they are binary safe
and support NULL characters, which means we are truncating the string at
the null character.
This is a breaking change for modules, so wanted to raise it for the
upcoming patch.
An alternative is we could expose the script code as a server object,
which would require some construction of the object in the function
pathway and freeing. I opted to keep the code flow as similar as
possible for the patch.
There is a test which only passes now.
For some reasons, functions don't support binary data. So as of right
now this patch doesn't fix that case.
Resolves https://github.com/valkey-io/valkey/issues/1942
---------
Signed-off-by: Madelyn Olson <[email protected]>
This change enhances user experience and consistency by allowing a
module to block a client on keyspace event notifications. Consistency is
improved by allowing that reads after writes on the same connection
yield expected results. For example, in ValkeySearch, mutations
processed earlier on the same connection will be available for search.
The implementation extends `VM_BlockClient` to support blocking clients
on keyspace event notifications. Internal clients, LUA clients, clients
issueing multi exec and those with the `deny_blocking` flag set are not
blocked. Once blocked, a client’s reply is withheld until it is
explicitly unblocked.
---------
Signed-off-by: yairgott <[email protected]>
### Issue https://github.com/valkey-io/valkey/issues/1175
### Problem
Performance degradation occurs due to the sanity
check([lookupCommandByCString](https://github.com/valkey-io/valkey/blob/unstable/src/module.c#L3539))
in the VM_Replicate function, which converts const char* into sds and
free them for command dictionary lookup. This check is mainly used for
debugging purpose, but it is unnecessary for trusted modules. The user
seeks a way to bypass this check for performance gains.
### Solution
Introduce a new SKIP_COMMAND_VALIDATION option which allows individual
modules to opt out of command validation
### Test
ADded a unit test
---------
Signed-off-by: Seungmin Lee <[email protected]>
Co-authored-by: Seungmin Lee <[email protected]>
* Pass `sender_id` as `NULL` terminated string as part of
`ValkeyModuleClusterMessageReceiver` for ease of usage by the module(s).
* Implement test coverage described in #1656 and ensures that nodes in
the cluster module properly acknowledge a "DING" message by sending a
"DONG" response.
Closes#1656.
---------
Signed-off-by: Nikhil Manglore <[email protected]>
CI report this failure:
```
ValueError: invalid literal for int() with base 10: ':100\r\n'
```
We were writing out a second response on the error.
Signed-off-by: Madelyn Olson <[email protected]>
This PR adds support for MustObeyClient Module API.
The purpose of this API is for Modules to handle commands / callbacks
knowing whether commands are arriving from the primary client or AOF
client and should never be rejected.
A use case is that Modules can have validation logic in command handlers
which only should be executed on primary nodes. Replica nodes must obey
commands replicated.
---------
Signed-off-by: KarthikSubbarao <[email protected]>
Signed-off-by: Karthik Subbarao <[email protected]>
In this PR we re-implement the `EVAL` commands (`EVAL`, `EVALSHA`,
`SCRIPT LOAD`, etc...) to use the scripting engine infrastructure
introduced in 6adef8e2f9. This allows
`EVAL` to run scripts using different scripting engines.
The Lua scripting engine implementation code was moved into its own
subdirectory `src/lua`.
This new implementation generalizes the module API for implementing
scripting engines to work with both `FUNCTION` and `EVAL` commands.
Module API changes include:
* Rename of callback
`ValkeyModuleScriptingEngineCreateFunctionsLibraryFunc` to
`ValkeyModuleScriptingEngineCompileCodeFunc`.
* Addition of a new enum `enum ValkeyModuleScriptingEngineSubsystemType`
to specify the scripting engine subsystem (EVAL, or FUNCTION, or both).
* In most callbacks was added a new parameter with the type
`ValkeyModuleScriptingEngineSubsystemType`.
* New callback specific for EVAL
`ValkeyModuleScriptingEngineResetEvalEnvFunc`.
* New API function `ValkeyModuleScriptingEngineExecutionState
(*ValkeyModule_GetFunctionExecutionState)(ValkeyModuleScriptingEngineServerRuntimeCtx
*server_ctx)` that is used by scripting engines to query the server
about the execution state of the script that is running.
Fixes#1261Fixes#1468
Follow-up of #1277
---------
Signed-off-by: Ricardo Dias <[email protected]>
Signed-off-by: Viktor Söderqvist <[email protected]>
Co-authored-by: Viktor Söderqvist <[email protected]>
Co-authored-by: Ping Xie <[email protected]>
This change uses the light message header for cluster module message
type to be sent to a target node or broadcast across the cluster. The
light message header was introduced in Valkey 8 to reduce network in/out
over clusterbus and have been already used for pub/sub message transfer.
It's only used if the target node supports the light message header
(~30B) otherwise it falls back to using the regular header (~2KB). The
module API `VM_SendClusterMessage` remains as is.
---------
Signed-off-by: Harkrishn Patro <[email protected]>
Signed-off-by: Harkrishn Patro <[email protected]>
Co-authored-by: Viktor Söderqvist <[email protected]>
This commit creates a new compilation unit for the scripting engine code
by extracting the existing code from the functions unit.
We're doing this refactor to prepare the code for running the `EVAL`
command using different scripting engines.
This PR has a module API change: we changed the type of error messages
returned by the callback
`ValkeyModuleScriptingEngineCreateFunctionsLibraryFunc` to be a
`ValkeyModuleString` (aka `robj`);
This PR also fixes#1470.
---------
Signed-off-by: Ricardo Dias <[email protected]>
When latency-monitor-threshold is set to 0, it means the latency monitor
is disabled, and in VM_LatencyAddSample, we wrote the condition
incorrectly, causing us to record latency when latency was turned off.
This bug was introduced in the very first day, see e3b1d6d, it was merged
in 2019.
Signed-off-by: Binbin <[email protected]>
This PR extends the module API to support the addition of different
scripting engines to execute user defined functions.
The scripting engine can be implemented as a Valkey module, and can be
dynamically loaded with the `loadmodule` config directive, or with the
`MODULE LOAD` command.
This PR also adds an example of a dummy scripting engine module, to show
how to use the new module API. The dummy module is implemented in
`tests/modules/helloscripting.c`.
The current module API support, only allows to load scripting engines to
run functions using `FCALL` command.
The additions to the module API are the following:
```c
/* This struct represents a scripting engine function that results from the
* compilation of a script by the engine implementation. */
struct ValkeyModuleScriptingEngineCompiledFunction
typedef ValkeyModuleScriptingEngineCompiledFunction **(*ValkeyModuleScriptingEngineCreateFunctionsLibraryFunc)(
ValkeyModuleScriptingEngineCtx *engine_ctx,
const char *code,
size_t timeout,
size_t *out_num_compiled_functions,
char **err);
typedef void (*ValkeyModuleScriptingEngineCallFunctionFunc)(
ValkeyModuleCtx *module_ctx,
ValkeyModuleScriptingEngineCtx *engine_ctx,
ValkeyModuleScriptingEngineFunctionCtx *func_ctx,
void *compiled_function,
ValkeyModuleString **keys,
size_t nkeys,
ValkeyModuleString **args,
size_t nargs);
typedef size_t (*ValkeyModuleScriptingEngineGetUsedMemoryFunc)(
ValkeyModuleScriptingEngineCtx *engine_ctx);
typedef size_t (*ValkeyModuleScriptingEngineGetFunctionMemoryOverheadFunc)(
void *compiled_function);
typedef size_t (*ValkeyModuleScriptingEngineGetEngineMemoryOverheadFunc)(
ValkeyModuleScriptingEngineCtx *engine_ctx);
typedef void (*ValkeyModuleScriptingEngineFreeFunctionFunc)(
ValkeyModuleScriptingEngineCtx *engine_ctx,
void *compiled_function);
/* This struct stores the callback functions implemented by the scripting
* engine to provide the functionality for the `FUNCTION *` commands. */
typedef struct ValkeyModuleScriptingEngineMethodsV1 {
uint64_t version; /* Version of this structure for ABI compat. */
/* Library create function callback. When a new script is loaded, this
* callback will be called with the script code, and returns a list of
* ValkeyModuleScriptingEngineCompiledFunc objects. */
ValkeyModuleScriptingEngineCreateFunctionsLibraryFunc create_functions_library;
/* The callback function called when `FCALL` command is called on a function
* registered in this engine. */
ValkeyModuleScriptingEngineCallFunctionFunc call_function;
/* Function callback to get current used memory by the engine. */
ValkeyModuleScriptingEngineGetUsedMemoryFunc get_used_memory;
/* Function callback to return memory overhead for a given function. */
ValkeyModuleScriptingEngineGetFunctionMemoryOverheadFunc get_function_memory_overhead;
/* Function callback to return memory overhead of the engine. */
ValkeyModuleScriptingEngineGetEngineMemoryOverheadFunc get_engine_memory_overhead;
/* Function callback to free the memory of a registered engine function. */
ValkeyModuleScriptingEngineFreeFunctionFunc free_function;
} ValkeyModuleScriptingEngineMethodsV1;
/* Registers a new scripting engine in the server.
*
* - `engine_name`: the name of the scripting engine. This name will match
* against the engine name specified in the script header using a shebang.
*
* - `engine_ctx`: engine specific context pointer.
*
* - `engine_methods`: the struct with the scripting engine callback functions
* pointers.
*/
int ValkeyModule_RegisterScriptingEngine(ValkeyModuleCtx *ctx,
const char *engine_name,
void *engine_ctx,
ValkeyModuleScriptingEngineMethods engine_methods);
/* Removes the scripting engine from the server.
*
* `engine_name` is the name of the scripting engine.
*
*/
int ValkeyModule_UnregisterScriptingEngine(ValkeyModuleCtx *ctx, const char *engine_name);
```
---------
Signed-off-by: Ricardo Dias <[email protected]>
Before Redis OSS 7, if we load a module with some arguments during
runtime,
and run the command "config rewrite", the module information will not be
saved into the
config file.
Since Redis OSS 7 and Valkey 7.2, if we load a module with some
arguments during runtime,
the module information (path, arguments number, and arguments value) can
be saved into the config file after config rewrite command is called.
Thus, the module will be loaded automatically when the server startup
next time.
Following is one example:
bind 172.25.0.58
port 7000
protected-mode no
enable-module-command yes
Generated by CONFIG REWRITE
latency-tracking-info-percentiles 50 99 99.9
dir "/home/ubuntu/valkey"
save 3600 1 300 100 60 10000
user default on nopass sanitize-payload ~* &* +https://github.com/ALL
loadmodule tests/modules/datatype.so 10 20
However, there is one problem.
If developers write a module, and update the running arguments by
someway, the updated arguments can not be saved into the config file
even "config rewrite" is called.
The reason comes from the following function
rewriteConfigLoadmoduleOption (src/config.c)
void rewriteConfigLoadmoduleOption(struct rewriteConfigState *state) {
..........
struct ValkeyModule *module = dictGetVal(de);
line = sdsnew("loadmodule ");
line = sdscatsds(line, module->loadmod->path);
for (int i = 0; i < module->loadmod->argc; i++) {
line = sdscatlen(line, " ", 1);
line = sdscatsds(line, module->loadmod->argv[i]->ptr);
}
rewriteConfigRewriteLine(state, "loadmodule", line, 1);
.......
}
The function only save the initial arguments information
(module->loadmod) into the configfile.
After core members discuss, ref
https://github.com/valkey-io/valkey/issues/1177
We decide add the following API to implement this feature:
Original proposal:
int VM_UpdateRunTimeArgs(ValkeyModuleCtx *ctx, int index, char *value);
Updated proposal:
ValkeyModuleString **values VM_GetRuntimeArgs(ValkeyModuleCtx *ctx);
**int VM_UpdateRuntimeArgs(ValkeyModuleCtx *ctx, int argc,
ValkeyModuleString **values);
Why we do not recommend the following way:
MODULE UNLOAD
Update module args in the conf file
MODULE LOAD
I think there are the following disadvantages:
1. Some modules can not be unloaded. Such as the example module
datatype.so, which is tests/modules/datatype.so
2. it is not atomic operation for MODULE UNLOAD + MODULE LOAD
3. sometimes, if we just run the module unload, the client business
could be interrupted
---------
Signed-off-by: hwware <[email protected]>
With this commit, users are able to build valkey using `CMake`.
## Example usage:
Build `valkey-server` in Release mode with TLS enabled and using
`jemalloc` as the allocator:
```bash
mkdir build-release
cd $_
cmake .. -DCMAKE_BUILD_TYPE=Release \
-DCMAKE_INSTALL_PREFIX=/tmp/valkey-install \
-DBUILD_MALLOC=jemalloc -DBUILD_TLS=1
make -j$(nproc) install
# start valkey
/tmp/valkey-install/bin/valkey-server
```
Build `valkey-unit-tests`:
```bash
mkdir build-release-ut
cd $_
cmake .. -DCMAKE_BUILD_TYPE=Release \
-DBUILD_MALLOC=jemalloc -DBUILD_UNIT_TESTS=1
make -j$(nproc)
# Run the tests
./bin/valkey-unit-tests
```
Current features supported by this PR:
- Building against different allocators: (`jemalloc`, `tcmalloc`,
`tcmalloc_minimal` and `libc`), e.g. to enable `jemalloc` pass
`-DBUILD_MALLOC=jemalloc` to `cmake`
- OpenSSL builds (to enable TLS, pass `-DBUILD_TLS=1` to `cmake`)
- Sanitizier: pass `-DBUILD_SANITIZER=<address|thread|undefined>` to
`cmake`
- Install target + redis symbolic links
- Build `valkey-unit-tests` executable
- Standard CMake variables are supported. e.g. to install `valkey` under
`/home/you/root` pass `-DCMAKE_INSTALL_PREFIX=/home/you/root`
Why using `CMake`? To list *some* of the advantages of using `CMake`:
- Superior IDE integrations: cmake generates the file
`compile_commands.json` which is required by `clangd` to get a compiler
accuracy code completion (in other words: your VScode will thank you)
- Out of the source build tree: with the current build system, object
files are created all over the place polluting the build source tree,
the best practice is to build the project on a separate folder
- Multiple build types co-existing: with the current build system, it is
often hard to have multiple build configurations. With cmake you can do
it easily:
- It is the de-facto standard for C/C++ project these days
More build examples:
ASAN build:
```bash
mkdir build-asan
cd $_
cmake .. -DBUILD_SANITIZER=address -DBUILD_MALLOC=libc
make -j$(nproc)
```
ASAN with jemalloc:
```bash
mkdir build-asan-jemalloc
cd $_
cmake .. -DBUILD_SANITIZER=address -DBUILD_MALLOC=jemalloc
make -j$(nproc)
```
As seen by the previous examples, any combination is allowed and
co-exist on the same source tree.
## Valkey installation
With this new `CMake`, it is possible to install the binary by running
`make install` or creating a package `make package` (currently supported
on Debian like distros)
### Example 1: build & install using `make install`:
```bash
mkdir build-release
cd $_
cmake .. -DCMAKE_INSTALL_PREFIX=$HOME/valkey-install -DCMAKE_BUILD_TYPE=Release
make -j$(nproc) install
# valkey is now installed under $HOME/valkey-install
```
### Example 2: create a `.deb` installer:
```bash
mkdir build-release
cd $_
cmake .. -DCMAKE_BUILD_TYPE=Release
make -j$(nproc) package
# ... CPack deb generation output
sudo gdebi -n ./valkey_8.1.0_amd64.deb
# valkey is now installed under /opt/valkey
```
### Example 3: create installer for non Debian systems (e.g. FreeBSD or
macOS):
```bash
mkdir build-release
cd $_
cmake .. -DCMAKE_BUILD_TYPE=Release
make -j$(nproc) package
mkdir -p /opt/valkey && ./valkey-8.1.0-Darwin.sh --prefix=/opt/valkey --exclude-subdir
# valkey-server is now installed under /opt/valkey
```
Signed-off-by: Eran Ifrah <[email protected]>
The reason is VM_Call will use a fake client without connection,
so we also need to check if c->conn is NULL.
This also affects scripts. If they are called in the script, the
server will crash. Injecting commands into AOF will also cause
startup failure.
Fixes#1054.
Signed-off-by: Binbin <[email protected]>
Scope of the changes:
- updated example modules to reference Valkey vs Redis in variable names
- updated tests to use valkeymodule.h
- updated vars in tests/modules to use valkey vs redis in variable names
Summary of the testing:
- ran make for all modules, loaded them into valkey-server and tested
commands
- ran make for test/modules
- ran make test for the entire codebase
---------
Signed-off-by: Dmitry Polyakovsky <[email protected]>
Co-authored-by: Dmitry Polyakovsky <[email protected]>
ValkeyModuleEvent_MasterLinkChange was updated to use more inclusive
language, but it was done in the compatibility layer as well
(RedisModuleEvent_).
---------
Signed-off-by: Roshan Khatri <[email protected]>
This includes comments used for module API documentation.
* Strategy for replacement: Regex search: `(//|/\*| \*|#).* ("|\()?(r|R)edis( |\.
|'|\n|,|-|\)|")(?!nor the names of its contributors)(?!Ltd.)(?!Labs)(?!Contributors.)`
* Don't edit copyright comments
* Replace "Redis version X.X" -> "Redis OSS version X.X" to distinguish
from newly licensed repository
* Replace "Redis Object" -> "Object"
* Exclude markdown for now
* Don't edit Lua scripting comments referring to redis.X API
* Replace "Redis Protocol" -> "RESP"
* Replace redis-benchmark, -cli, -server, -check-aof/rdb with "valkey-"
prefix
* Most other places, I use best judgement to either remove "Redis", or
replace with "the server" or "server"
Fixes#148
---------
Signed-off-by: Jacob Murphy <[email protected]>
Signed-off-by: Viktor Söderqvist <[email protected]>
This commit updates the following fields:
1. server_version -> valkey_version in server info. Since we would like
to advertise specific compatibility, we are making the version specific
to valkey. servername will remain as an optional indicator, and other
valkey compatible stores might choose to advertise something else.
1. We dropped redis-ver from the API. This isn't related to API
compatibility, but we didn't want to "fake" that valkey was creating an
rdb from a Redis version.
1. Renamed server-ver -> valkey_version in rdb info. Same as point one,
we want to explicitly indicate this was created by a valkey server.
---------
Signed-off-by: Madelyn Olson <[email protected]>
Sometimes it's useful to compute a key's cluster slot in a module.
This API function is just like the command CLUSTER KEYSLOT (but faster).
A "reverse" API is also added:
`RedisModule_ClusterCanonicalKeyNameInSlot`. Given a slot, it returns a
short string that we can call a canonical key for the slot.
The block timeout is passed in the test case, but we do not pass
in the timeout_callback, and it will crash when unlocking. In this
case, in moduleBlockedClientTimedOut we will check timeout_callback.
There is the stack:
```
beforeSleep
blockedBeforeSleep
handleBlockedClientsTimeout
checkBlockedClientTimeout
unblockClientOnTimeout
replyToBlockedClientTimedOut
moduleBlockedClientTimedOut
-- timeout_callback is NULL, invalidFunctionWasCalled
bc->timeout_callback(&ctx,(void**)c->argv,c->argc);
```
Modules may want to handle allocation failures gracefully. Adding
RM_TryCalloc() and RM_TryRealloc() for it.
RM_TryAlloc() was added before:
https://github.com/redis/redis/pull/10541
Fix#12785 and other race condition issues.
See the following isolated comments.
The following report was obtained using SANITIZER thread.
```sh
make SANITIZER=thread
./runtest-moduleapi --config io-threads 4 --config io-threads-do-reads yes --accurate
```
1. Fixed thread-safe issue in RM_UnblockClient()
Related discussion:
https://github.com/redis/redis/pull/12817#issuecomment-1831181220
* When blocking a client in a module using `RM_BlockClientOnKeys()` or
`RM_BlockClientOnKeysWithFlags()`
with a timeout_callback, calling RM_UnblockClient() in module threads
can lead to race conditions
in `updateStatsOnUnblock()`.
- Introduced:
Version: 6.2
PR: #7491
- Touch:
`server.stat_numcommands`, `cmd->latency_histogram`, `server.slowlog`,
and `server.latency_events`
- Harm Level: High
Potentially corrupts the memory data of `cmd->latency_histogram`,
`server.slowlog`, and `server.latency_events`
- Solution:
Differentiate whether the call to moduleBlockedClientTimedOut() comes
from the module or the main thread.
Since we can't know if RM_UnblockClient() comes from module threads, we
always assume it does and
let `updateStatsOnUnblock()` asynchronously update the unblock status.
* When error reply is called in timeout_callback(), ctx is not
thread-safe, eventually lead to race conditions in `afterErrorReply`.
- Introduced:
Version: 6.2
PR: #8217
- Touch
`server.stat_total_error_replies`, `server.errors`,
- Harm Level: High
Potentially corrupts the memory data of `server.errors`
- Solution:
Make the ctx in `timeout_callback()` with `REDISMODULE_CTX_THREAD_SAFE`,
and asynchronously reply errors to the client.
2. Made RM_Reply*() family API thread-safe
Related discussion:
https://github.com/redis/redis/pull/12817#discussion_r1408707239
Call chain: `RM_Reply*()` -> `_addReplyToBufferOrList()` -> touch
server.current_client
- Introduced:
Version: 7.2.0
PR: #12326
- Harm Level: None
Since the module fake client won't have the `CLIENT_PUSHING` flag, even
if we touch server.current_client,
we can still exit after `c->flags & CLIENT_PUSHING`.
- Solution
Checking `c->flags & CLIENT_PUSHING` earlier.
3. Made freeClient() thread-safe
Fix#12785
- Introduced:
Version: 4.0
Commit:
https://github.com/redis/redis/commit/3fcf959e609e850a114d4016843e4c991066ebac
- Harm Level: Moderate
* Trigger assertion
It happens when the module thread calls freeClient while the io-thread
is in progress,
which just triggers an assertion, and doesn't make any race condiaions.
* Touch `server.current_client`, `server.stat_clients_type_memory`, and
`clientMemUsageBucket->clients`.
It happens between the main thread and the module threads, may cause
data corruption.
1. Error reset `server.current_client` to NULL, but theoretically this
won't happen,
because the module has already reset `server.current_client` to old
value before entering freeClient.
2. corrupts `clientMemUsageBucket->clients` in
updateClientMemUsageAndBucket().
3. Causes server.stat_clients_type_memory memory statistics to be
inaccurate.
- Solution:
* No longer counts memory usage on fake clients, to avoid updating
`server.stat_clients_type_memory` in freeClient.
* No longer resetting `server.current_client` in unlinkClient, because
the fake client won't be evicted or disconnected in the mid of the
process.
* Judgment assertion `io_threads_op == IO_THREADS_OP_IDLE` only if c is
not a fake client.
4. Fixed free client args without GIL
Related discussion:
https://github.com/redis/redis/pull/12817#discussion_r1408706695
When freeing retained strings in the module thread (refcount decr), or
using them in some way (refcount incr), we should do so while holding
the GIL,
otherwise, they might be simultaneously freed while the main thread is
processing the unblock client state.
- Introduced:
Version: 6.2.0
PR: #8141
- Harm Level: Low
Trigger assertion or double free or memory leak.
- Solution:
Documenting that module API users need to ensure any access to these
retained strings is done with the GIL locked
5. Fix adding fake client to server.clients_pending_write
It will incorrectly log the memory usage for the fake client.
Related discussion:
https://github.com/redis/redis/pull/12817#issuecomment-1851899163
- Introduced:
Version: 4.0
Commit:
https://github.com/redis/redis/commit/9b01b64430fbc1487429144d2e4e72a4a7fd9db2
- Harm Level: None
Only result in NOP
- Solution:
* Don't add fake client into server.clients_pending_write
* Add c->conn assertion for updateClientMemUsageAndBucket() and
updateClientMemoryUsage() to avoid same
issue in the future.
So now it will be the responsibility of the caller of both of them to
avoid passing in fake client.
6. Fix calling RM_BlockedClientMeasureTimeStart() and
RM_BlockedClientMeasureTimeEnd() without GIL
- Introduced:
Version: 6.2
PR: #7491
- Harm Level: Low
Causes inaccuracies in command latency histogram and slow logs, but does
not corrupt memory.
- Solution:
Module API users, if know that non-thread-safe APIs will be used in
multi-threading, need to take responsibility for protecting them with
their own locks instead of the GIL, as using the GIL is too expensive.
### Other issue
1. RM_Yield is not thread-safe, fixed via #12905.
### Summarize
1. Fix thread-safe issues for `RM_UnblockClient()`, `freeClient()` and
`RM_Yield`, potentially preventing memory corruption, data disorder, or
assertion.
2. Updated docs and module test to clarify module API users'
responsibility for locking non-thread-safe APIs in multi-threading, such
as RM_BlockedClientMeasureTimeStart/End(), RM_FreeString(),
RM_RetainString(), and RM_HoldString().
### About backpot to 7.2
1. The implement of (1) is not too satisfying, would like to get more
eyes.
2. (2), (3) can be safely for backport
3. (4), (6) just modifying the module tests and updating the
documentation, no need for a backpot.
4. (5) is harmless, no need for a backpot.
---------
Co-authored-by: Oran Agra <[email protected]>
