Rebranding of code in libvalkeycluster #15
Conversation
Signed-off-by: Björn Svensson <[email protected]>
Signed-off-by: Björn Svensson <[email protected]>
Signed-off-by: Björn Svensson <[email protected]>
libvalkeycluster/valkeycluster.h
Outdated
| #define VALKEYCLUSTER_SLOTS 16384 | ||
|
|
||
| #define VALKEY_ROLE_NULL 0 | ||
| #define VALKEY_ROLE_MASTER 1 |
There was a problem hiding this comment.
Can we change the terminology?
There was a problem hiding this comment.
Sure, the master/slave terminology should be removed
There was a problem hiding this comment.
Create follow-up issue?
libvalkeycluster/valkeycluster.h
Outdated
| int valkeyClusterUpdateSlotmap(valkeyClusterContext *cc); | ||
|
|
||
| /* Internal functions */ | ||
| valkeyContext *ctx_get_by_node(valkeyClusterContext *cc, |
There was a problem hiding this comment.
Hmmm I wonder if internal functions should really be in a public header. Maybe they could/should be moved to a separate header?
There was a problem hiding this comment.
In the renaming branch I renamed them, and kept them public.
It currently needs to be public to handle PUB/SUB in cluster:
Nordix/hiredis-cluster#166 (comment)
Nordix/hiredis-cluster#143 (comment)
Signed-off-by: Björn Svensson <[email protected]>
Makefiles and tests are moved separately
|
The adapter .h files include #include "../valkeycluster.h"
#include <valkey/adapters/glib.h>I.e. they're cluster event loop adapters that depend on the corresponding non-cluster adapters. But the file name is just |
|
`What's the idea? Merge the files with non-cluster ones or have two separate ones? Oh, good finding. I'll need to update the includes after moving stuff to src/. The plan was to have a single adapter sourcefile for each eventsystem, i.e. basically the version existing in libvalkey but with the added attach function from libvalkeycluster, like |
|
Maybe its simpler in this stage to not rename and move the cluster adapters. |
|
Yes. What matters in the end is how they are used by users. #include <valkey/cluster-async.h>
#include <valkey/cluster-adapters/glib.h>Or can we make it work without cluster-specific adapters? #include <valkey/cluster-async.h>
#include <valkey/adapters/glib.h> |
This is the plan, and it seems to work. |
|
Rebased and updated the includes for the moved cluster files. |
| redisClusterAsyncContext *cc = | ||
| redisClusterAsyncConnect("127.0.0.1:7000", HIRCLUSTER_FLAG_NULL); | ||
| valkeyClusterAsyncContext *cc = | ||
| valkeyClusterAsyncConnect("127.0.0.1:7000", VALKEYCLUSTER_FLAG_NULL); |
There was a problem hiding this comment.
it's just 0? Can we rename this to VALKEYCLUSTER_FLAG_NONE or just use 0? "Null" sticks out to me.
There was a problem hiding this comment.
I think it would be nice to replace the flags used in the Cluster API with a new valkeyClusterOptions,
like its done in standalone:
valkeyAsyncContext *valkeyAsyncConnectWithOptions(const valkeyOptions *options);
Then we could set any option with that API, and possibly keep the simpler one for easy usage.
valkeyClusterAsyncContext *valkeyClusterAsyncConnectWithOptions(const valkeyClusterOptions *options);
valkeyClusterAsyncContext *valkeyClusterAsyncConnect(const char *addrs);
With this change the cluster api would look the same as the standalone.
Presumes that libvalkey (old hiredis) is already renamed as in branch renaming.
Part of #11