Introduce configuration options in the cluster API #137

bjosv · 2024-12-04T22:00:42Z

The standalone API uses the options struct valkeyOptions to allow users to configure how libvalkey set up the context.
This struct is used when calling valkeyConnectWithOptions(valkeyOptions *opt), which returns a connected context with prepared command timeout values and so on.

The current cluster API, which in inherited from the project hiredis-vip and hiredis-cluster, has some similarities to the standalone API, but it does not provide a options struct and does not have the same "look and feel" as the standalone API.

This PR introduces a valkeyClusterOption that allows a user to set all options before connecting to the cluster using:
valkeyClusterContext *cc = valkeyClusterConnectWithOptions(&options)
or
valkeyClusterAsyncContext *acc = valkeyClusterAsyncConnectWithOptions(&options) when using the async API.

This options struct enables users to set the initial nodes, callbacks, timeouts, options, TLS- and event-adapters,
instead of using a wide range of valkeyClusterSetOptionXxx functions on a valkeyClusterContext (even when they use async contexts valkeyClusterAsyncContext...).
There are still options that are allowed to be configured during runtime likevalkeyClusterSetOptionTimeout, but most other options requires a reconnect.

The async cluster API will no longer require that users calls the blocking cluster API using valkeyClusterConnect2(acc->cc).
We now provide a new option VALKEY_OPT_BLOCKING_INITIAL_UPDATE to enable a blocking slotmap update after an initial connect using the valkeyClusterAsyncConnectWithOptions API. This will allow us to avoid acc->cc usages and we should be able to hide the cc in the acc struct.

All testcases are updated to use the new API and this should give a feeling how it works,
alternatively look at changes in include/valkey/cluster.h.
The migration guide and docs are not yet updated since its a draft PR.

zuiderkwast

The API looks good in general!

include/valkey/adapters/libevent.h

include/valkey/cluster_tls.h

include/valkey/cluster.h

These flags will be replaced by options and can be set using these new APIs instead: - valkeyClusterConnectWithOptions - valkeyClusterAsyncConnectWithOptions Signed-off-by: Björn Svensson <[email protected]>

Signed-off-by: Björn Svensson <[email protected]>

zuiderkwast

Finally made a pass on this PR. Looks good, but can't we remove more connect variants? It seems like connect-with-options is the only connect API we need?

docs/cluster.md

src/cluster.c

zuiderkwast · 2025-01-28T16:31:42Z

src/cluster.c

@@ -2823,64 +2792,76 @@ valkeyClusterGetValkeyAsyncContext(valkeyClusterAsyncContext *acc,
    return ac;
 }

-valkeyClusterAsyncContext *valkeyClusterAsyncContextInit(void) {
+valkeyClusterAsyncContext *valkeyClusterAsyncContextInit(const valkeyClusterOptions *options) {


Is there any point in supporting init without connect?

valkeyClusterAsyncConnectWithOptions would be enough, wouldn't it?

Hm.... is it event loop related? The event loop needs to be attache between init and connect?

src/cluster.c

include/valkey/cluster.h

The option VALKEY_OPT_USE_CLUSTER_NODES can be used to change to `cluster nodes`. Signed-off-by: Björn Svensson <[email protected]>

Signed-off-by: Björn Svensson <[email protected]>

This change makes cluster_tls.[c.h] files superfluous. Signed-off-by: Björn Svensson <[email protected]>

Signed-off-by: Björn Svensson <[email protected]>

Use similar naming as option `connect_callback` for the blocking API. Signed-off-by: Björn Svensson <[email protected]>

Signed-off-by: Björn Svensson <[email protected]>

zuiderkwast

Looks mostly great. A large change that isn't very well explained in the docs is that you now attach the event loop to the options, instead of to the acc. We need to improve this part, also in the migration guide because it's an important and non-trivial change.

zuiderkwast · 2025-01-30T10:48:55Z

docs/cluster.md

+opt.password = "password"                            // Authentication; libvalkey sends the `AUTH` command.
+
+// Enable TLS using a prepared `valkeyTLSContext` when connecting.
+options.tls = tlsCtx;


The valkeyClusterOptions variable is called opt in this example, not options.

zuiderkwast · 2025-01-30T10:51:53Z

docs/cluster.md

-    // handle error
+valkeyClusterOptions opt = {0};
+opt.initial_nodes = "127.0.0.1:6379,127.0.0.1:6380"; // Addresses to initially connect to.
+opt.options = VALKEY_OPT_USE_CLUSTER_NODES;          // See available flags below.


It's a bit odd to have an options field in the options struct. Is it a big change for users if we rename it to flags? Matching standalone options, etc.

I used the name options for this field to match how it's named in valkeyOptions (which also has an options which takes VALKEY_OPT_*.). I guess we should change both in that case.

When contexts are initiated we keep the given option-flags in a context-field named flags, but here we also use it to keep internal state, both in cluster contexts and standalone.

We can keep options then. If we change to flags, it would imply all the macro names that contain VALKEY_OPT too. It would be too big change maybe.

Yes, it getting large then. We can think about it, and possibly replace them all in a separate PR afterwards.

zuiderkwast · 2025-01-30T10:52:43Z

docs/cluster.md

+
+| Flag | Description  |
+| --- | --- |
+| VALKEY\_OPT\_USE\_CLUSTER\_NODES | Tells libvalkey to use the command `CLUSTER NODES` when updating its slot map (cluster topology).<br>Libvalkey uses `CLUSTER SLOTS` by default. |


If you use backticks around these constants, you don't need to escape the underscore.

ah nice! That makes it readable when "raw-reading" from a terminal, will update standalone.md too.

zuiderkwast · 2025-01-30T10:59:48Z

docs/migration-guide.md

@@ -27,6 +27,9 @@ The type `sds` is removed from the public API.

 ## Migrating from `hiredis-cluster` 0.14.0

+The command used to update the internal slot map is changed to `CLUSTER SLOTS`.


Suggested change

The command used to update the internal slot map is changed to `CLUSTER SLOTS`.

The default command to update the internal slot map is changed to `CLUSTER SLOTS`.

zuiderkwast · 2025-01-30T11:22:45Z

docs/cluster.md

@@ -194,18 +235,7 @@ The first alternative is to use the function `valkeyClusterAsyncConnect`, which
 Any command sent by the user thereafter will create a new non-blocking connection, unless a non-blocking connection already exists to the destination.


(I can't comment on the lines above.) The async API doc starts like this:

Asynchronous API

Connecting

There are two alternative ways to initiate a cluster client, which also determines how the client behaves during the initial connect.
The first alternative is to use the function valkeyClusterAsyncConnect, which initially connects to the cluster in a blocking fashion and waits for the slot map before returning.

... and then the variant with init + connect2 is described.

But I checked cluster.h and I think we deleted the blocking connect and renamed connect2 to connect. We don't have any blocking valkeyClusterAsyncConnect anymore, do we?

I think the doc should first describe this variant:

Create options struct

Attach event loop to options

valkeyClusterAsyncConnectWithOptions

valkeyClusterOptions options = { .initial_nodes = "127.0.0.1:7000"; }; valkeyClusterOptionsUseLibev(&options, EV_DEFAULT); valkeyClusterAsyncContext *acc = valkeyClusterAsyncConnectWithOptions(&options);

and then the second variant with one more step acc = init(options); connect(acc).

Oh, this part doesn't seem finished, there are a lot of empty lines too :( I will update.

zuiderkwast · 2025-01-30T11:41:45Z

docs/migration-guide.md

+* The cluster client initiation procedure is changed and `valkeyClusterOptions`
+  should be used to specify options when creating a context.
+  The `examples` directory in this repository contains some common client
+  initiation examples that might be helpful.


We can also link to cluster.md from this migration guide, right? I think cluster.md should have some example of async connect with options.

We should be able to link just like [Link](cluster.md#section)

bjosv requested a review from zuiderkwast December 4, 2024 22:00

zuiderkwast reviewed Dec 6, 2024

View reviewed changes

include/valkey/adapters/libevent.h Outdated Show resolved Hide resolved

include/valkey/cluster_tls.h Outdated Show resolved Hide resolved

bjosv force-pushed the clusterconfig branch from 29de378 to 8c31a25 Compare December 13, 2024 13:57

zuiderkwast reviewed Dec 17, 2024

View reviewed changes

include/valkey/cluster.h Outdated Show resolved Hide resolved

bjosv force-pushed the clusterconfig branch from 305a025 to a0cd5ff Compare January 21, 2025 22:58

bjosv added 6 commits January 22, 2025 00:43

Remove flags from cluster connect APIs

554481a

These flags will be replaced by options and can be set using these new APIs instead: - valkeyClusterConnectWithOptions - valkeyClusterAsyncConnectWithOptions Signed-off-by: Björn Svensson <[email protected]>

Add cluster connect with options

188e9ab

Signed-off-by: Björn Svensson <[email protected]>

Add async cluster connect with options

0403b39

Signed-off-by: Björn Svensson <[email protected]>

Remove config functions that are replaced with options

c582f07

Signed-off-by: Björn Svensson <[email protected]>

New option: Blocking slotmap updates after initial async connect

b5ad0a9

Signed-off-by: Björn Svensson <[email protected]>

Update documentation

b61a813

Signed-off-by: Björn Svensson <[email protected]>

bjosv force-pushed the clusterconfig branch from a0cd5ff to b61a813 Compare January 21, 2025 23:52

fixup: correcting ct_out_of_memory_handling

978e2e1

Signed-off-by: Björn Svensson <[email protected]>

bjosv marked this pull request as ready for review January 22, 2025 10:24

zuiderkwast reviewed Jan 28, 2025

View reviewed changes

bjosv added 11 commits January 29, 2025 11:29

Merge remote-tracking branch 'upstream/main' into clusterconfig

aaa4eca

Use CLUSTER SLOTS during slotmap updates by default

a1f704c

The option VALKEY_OPT_USE_CLUSTER_NODES can be used to change to `cluster nodes`. Signed-off-by: Björn Svensson <[email protected]>

fixup: remove valkeyClusterOptionsSetEventCallback

3681d88

Signed-off-by: Björn Svensson <[email protected]>

fixup: remote valkeyClusterOptionsSetConnectCallback

8473801

Signed-off-by: Björn Svensson <[email protected]>

fixup: fix cluster.md review comments

a5ecc02

Signed-off-by: Björn Svensson <[email protected]>

fixup: remove helper valkeyClusterOptionsEnableTLS

bf83e65

This change makes cluster_tls.[c.h] files superfluous. Signed-off-by: Björn Svensson <[email protected]>

fixup: Add 'bitwise' to spellcheck wordlist in CI

6c88335

Signed-off-by: Björn Svensson <[email protected]>

fixup: fix leak warnings during OOM testing

97e02b6

Signed-off-by: Björn Svensson <[email protected]>

Update migration guide

28f790b

Signed-off-by: Björn Svensson <[email protected]>

Update docs and option name async_connect_callback

03b2aaf

Use similar naming as option `connect_callback` for the blocking API. Signed-off-by: Björn Svensson <[email protected]>

fixup: spelling correction

fa7c35d

Signed-off-by: Björn Svensson <[email protected]>

zuiderkwast reviewed Jan 30, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce configuration options in the cluster API #137

Introduce configuration options in the cluster API #137

bjosv commented Dec 4, 2024

zuiderkwast left a comment

zuiderkwast left a comment

zuiderkwast Jan 28, 2025

zuiderkwast Jan 28, 2025

zuiderkwast left a comment

zuiderkwast Jan 30, 2025

zuiderkwast Jan 30, 2025

bjosv Jan 30, 2025

zuiderkwast Jan 30, 2025

bjosv Jan 30, 2025

zuiderkwast Jan 30, 2025

bjosv Jan 30, 2025

zuiderkwast Jan 30, 2025

zuiderkwast Jan 30, 2025

bjosv Jan 30, 2025

zuiderkwast Jan 30, 2025 •

edited

Loading

		@@ -27,6 +27,9 @@ The type `sds` is removed from the public API.

		## Migrating from `hiredis-cluster` 0.14.0

		The command used to update the internal slot map is changed to `CLUSTER SLOTS`.

	The command used to update the internal slot map is changed to `CLUSTER SLOTS`.
	The default command to update the internal slot map is changed to `CLUSTER SLOTS`.

		@@ -194,18 +235,7 @@ The first alternative is to use the function `valkeyClusterAsyncConnect`, which
		Any command sent by the user thereafter will create a new non-blocking connection, unless a non-blocking connection already exists to the destination.

Introduce configuration options in the cluster API #137

Are you sure you want to change the base?

Introduce configuration options in the cluster API #137

Conversation

bjosv commented Dec 4, 2024

zuiderkwast left a comment

Choose a reason for hiding this comment

zuiderkwast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuiderkwast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Asynchronous API

Connecting

Choose a reason for hiding this comment

zuiderkwast Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

zuiderkwast Jan 30, 2025 •

edited

Loading