Remove REDISMODULE_ prefixes and introduce compatibility header by PingXie · Pull Request #194 · valkey-io/valkey

PingXie · 2024-04-04T05:11:40Z

Removed REDISMODULE_ prefixes from the core source code to align with the new SERVERMODULE_ naming convention. Added a new 'redismodule.h' header file to ensure full backward compatibility with existing modules. This compatibility layer maps all legacy REDISMODULE_ prefixed identifiers to their new SERVERMODULE_ equivalents, allowing existing Redis modules to function without modification.

Removed REDISMODULE_ prefixes from the core source code to align with the new SERVERMODULE_ naming convention. Added a new 'redismodule.h' header file to ensure full backward compatibility with existing modules. This compatibility layer maps all legacy REDISMODULE_ prefixed identifiers to their new SERVERMODULE_ equivalents, allowing existing Redis modules to function without modification. Signed-off-by: Ping Xie <pingxie@google.com>

PingXie · 2024-04-04T05:21:28Z

One high level question.

Should we use SERVERMODULE_* or VALKEYMODULE_*? I have been using the generic "server" term everywhere in this PR, including both file names and APIs. Now on a second thought, I feel that "VALKEYMODULE_" would probably work better. I am leaving the PR as-is for now but curious to hear the thoughts from others.

natoscott · 2024-04-04T06:49:42Z

+1 for VALKEYMODULE_ prefix.

zuiderkwast · 2024-04-04T09:34:41Z

Yeah, this header file is usually copied to the module's repo. Using a generic name like "servermodule" is not appropriate in this context, i.e. outside of valkey.

When documenting this, we should somehow explain that the valkeymodule API is a superset of redismodule API as it was in redis 7.2. I believe it can be confusing to module authors, especially if they want to be compatible with various forks at the same time.

madolson · 2024-04-04T18:03:38Z

+1 for VALKEYMODULE_ prefix.

I also agree with this. I think server is fine when we're just talking about agnostic stuff, but this is the ValkeyModule system now.

PingXie · 2024-04-04T19:26:50Z

SGTM. Will align to "valkeymodule"

Signed-off-by: Ping Xie <pingxie@google.com>

madolson

The defines mostly look correct to me. I'm a bit concerned that this is touching a lot of unnecessary files we'll need to hit on the backport (mainly the module .c files).

madolson · 2024-04-05T00:24:02Z

+/*
+ * valkeymodule.h
+ *
+ * This header file is renamed from valkeymodule.h to reflect the new naming conventions adopted in Valkey.


Seems like an outdated comment.

Seems like an outdated comment.

lol. I added it on purpose to explain why this file exists in the first place and how it is supposed to be used. I have tried hard but I just can't seem to imagine what context a first time valkey module writer who happens to be a veteran redis module developer would need when they see this file. Maybe I should move this comment into the new redismodule.h instead?

I think it's fine, I think it should just have been "This header file is renamed from redismodule.h". However, I think just omitting this comment and we can document it later makes sense.

PingXie · 2024-04-05T01:14:46Z

he defines mostly look correct to me. I'm a bit concerned that this is touching a lot of unnecessary files we'll need to hit on the backport (mainly the module .c files).

Agreed and this is still work in progress. @zuiderkwast any concern with moving this out of our 7.2.4 release and into the next one? I will continue patching up the remaining "redis" references.

source-compatible with the existing Redis modules but still **not** ABI compatible yet. Signed-off-by: Ping Xie <pingxie@google.com>

Signed-off-by: Ping Xie <pingxie@google.com>

PingXie · 2024-04-05T05:30:01Z

All changes are in. With this PR, we should be both ABI and source compatible with existing Redis modules. New modules can be written using Valkey only interfaces.

test-sanitizer-address is complaining about memory leaks. Will take a look tomorrow.

zuiderkwast · 2024-04-05T07:17:06Z

The defines mostly look correct to me. I'm a bit concerned that this is touching a lot of unnecessary files we'll need to hit on the backport (mainly the module .c files).

If it gets hard to backport this change, then as a backup plan we can include it in our next major version instead. It's not that much of a deal. We'll still have "Redis" in log entries and error replies in our 7.2.4 version.

zuiderkwast · 2024-04-05T13:33:16Z

@zuiderkwast any concern with moving this out of our 7.2.4 release and into the next one?

No concern with that. It's more safe to postpone it actually.

madolson

The implementation makes theoretical sense to me for module compatibility. The only thing I found missing was the unload module hook, I would be okay to fast follow that in a separate PR, there are also no existing modules that use it outside of the TLS compatibility one.

madolson · 2024-04-05T18:41:54Z

@@ -33,8 +33,8 @@
 * The comments in this file are used to generate the API documentation on the


This file is far too large to realistically review right now. I'm going to assume everything in this file was just a find/replacement outside of the few strategic compatibility hooks.

load and unload hooks are updated. Others are indeed just find/replace. Maybe I should run clang-format on module.c now? :-)

Eh, we're completely destroying git history, we can do it later and destroy it again.

madolson · 2024-04-05T18:45:30Z


-#endif /* REDISMODULE_CORE */
-#endif /* REDISMODULE_H */
+#include "valkeymodule.h"


Just a note about when we write compatibility. This will not always work, if someone was just copying the file and actually using it for codegen or something. AFAIK the redismodule-rs does this on the .h file.

Not a big deal IMO. Users can copy it from redis 7.2.4 instead.

I am also less concerned about this. the (build) errors are self-explanatory IMO. I also left sufficient comments in redismodule.h to explain its purpose. Lastly, I would expect module developers to use the redismodule.h copy from the redis tree to build redis modules.

zuiderkwast · 2024-04-05T19:32:54Z

We have to think about redis version vs valkey version. The API contains REDISMODULE_COMMAND_INFO_VERSION, which is used in some APIs. We may want to track both versions somehow.... or do we? (This one is only the versioning of a specific struct, I think...)

madolson · 2024-04-05T19:35:39Z

We may want to track both versions somehow.... or do we?

I honestly think we should introduce a "good" API tracking system for modules, unlike what is used today. I think a lot of modules use the version number to determine which APIs are present which I don't really like.

PingXie · 2024-04-05T19:57:30Z

I honestly think we should introduce a "good" API tracking system for modules, unlike what is used today. I think a lot of modules use the version number to determine which APIs are present which I don't really like.

Agreed. I don't think we should use API version at all. These module APIs are no different than the commands like "HGET" etc. They should be evolved independently with full backward compatibility.

PingXie · 2024-04-05T20:00:21Z

The implementation makes theoretical sense to me for module compatibility. The only thing I found missing was the unload module hook, I would be okay to fast follow that in a separate PR, there are also no existing modules that use it outside of the TLS compatibility one.

I will take a quick look into the unload. I have also narrowed the memory leak issue to a minimum repro.

zuiderkwast · 2024-04-05T21:35:05Z

I honestly think we should introduce a "good" API tracking system for modules, unlike what is used today. I think a lot of modules use the version number to determine which APIs are present which I don't really like.

Agreed. I don't think we should use API version at all. These module APIs are no different than the commands like "HGET" etc. They should be evolved independently with full backward compatibility.

Yeah, but even if we do introduce a better method, some modules will still use RedisModule_GetServerVersion(). Do we want it to return the redis version (7.2.4) or the valkey version? If we choose the latter, it can be confusing for modules what a number like 8.2.0 means.... So we can freeze it to 7.2.4 maybe?

madolson · 2024-04-05T22:13:47Z

So we can freeze it to 7.2.4 maybe?

I think we can decide this later. For now we should and will return 7.2.4.

Signed-off-by: Ping Xie <pingxie@google.com>

PingXie · 2024-04-05T22:47:12Z

The implementation makes theoretical sense to me for module compatibility. The only thing I found missing was the unload module hook, I would be okay to fast follow that in a separate PR, there are also no existing modules that use it outside of the TLS compatibility one.

I will take a quick look into the unload. I have also narrowed the memory leak issue to a minimum repro.

turns out that the unload hook is the reason of the leaks :-).

I think this PR is almost ready. Addressing the remaining comments...

Signed-off-by: Ping Xie <pingxie@google.com>

madolson

Just reviewed the diff, and LGTM

zuiderkwast · 2024-04-06T07:24:39Z

+ * This header file is forked from redismodule.h to reflect the new naming conventions adopted in Valkey.
+ * 
+ * Key Changes:
+ * - Symbolic names have been changed from VALKEYMODULE_* to VALKEYMODULE_* to align with the


From what to what? Fixme

zuiderkwast · 2024-04-06T07:27:56Z

+VALKEYMODULE_API const char *(*ValkeyModule_GetCurrentCommandName)(ValkeyModuleCtx *ctx) VALKEYMODULE_ATTR;
+VALKEYMODULE_API int (*ValkeyModule_RegisterDefragFunc)(ValkeyModuleCtx *ctx, ValkeyModuleDefragFunc func) VALKEYMODULE_ATTR;
+VALKEYMODULE_API void *(*ValkeyModule_DefragAlloc)(ValkeyModuleDefragCtx *ctx, void *ptr) VALKEYMODULE_ATTR;
+VALKEYMODULE_API ValkeyModuleString *(*ValkeyModule_DefragModuleString)(ValkeyModuleDefragCtx *ctx, ValkeyModuleString *str) VALKEYMODULE_ATTR;


DefragValkeyModuleString please. That's what it defrags.

…ey-io#194) Fix valkey-io#146 Removed REDISMODULE_ prefixes from the core source code to align with the new SERVERMODULE_ naming convention. Added a new 'redismodule.h' header file to ensure full backward compatibility with existing modules. This compatibility layer maps all legacy REDISMODULE_ prefixed identifiers to their new SERVERMODULE_ equivalents, allowing existing Redis modules to function without modification. --------- Signed-off-by: Ping Xie <pingxie@google.com>

madolson reviewed Apr 4, 2024

View reviewed changes

Comment thread src/servermodule.h Outdated

madolson reviewed Apr 4, 2024

View reviewed changes

Comment thread src/servermodule.h Outdated

Ping Xie added 3 commits April 4, 2024 15:49

Replace SERVERMODEUL_ with VALKEYMODULE_

87f6b4d

Signed-off-by: Ping Xie <pingxie@google.com>

Add a file header to valkeymodule.h to explain the changes

4f9350e

Signed-off-by: Ping Xie <pingxie@google.com>

Patch remaining files to use the new VALKEYMODULE_* symbols

51f1a06

Signed-off-by: Ping Xie <pingxie@google.com>

PingXie mentioned this pull request Apr 4, 2024

Register RedisModule_* APIs under the ValkeyModule_* namespace as well #195

Closed

madolson reviewed Apr 5, 2024

View reviewed changes

Ping Xie added 2 commits April 4, 2024 21:40

Remove all remaining "redis" references from valkeymodule.h. We are

ed58982

source-compatible with the existing Redis modules but still **not** ABI compatible yet. Signed-off-by: Ping Xie <pingxie@google.com>

Add ABI compat support for legacy Redis module binaries

8caa905

Signed-off-by: Ping Xie <pingxie@google.com>

madolson approved these changes Apr 5, 2024

View reviewed changes

Add support to handle RedisModule_OnUnload as well

fe4c804

Signed-off-by: Ping Xie <pingxie@google.com>

Incorporate review feedback

e8f0364

Signed-off-by: Ping Xie <pingxie@google.com>

madolson approved these changes Apr 5, 2024

View reviewed changes

PingXie merged commit aaec321 into valkey-io:unstable Apr 5, 2024

zuiderkwast reviewed Apr 6, 2024

View reviewed changes

madolson mentioned this pull request Apr 6, 2024

Rename ValkeyModule_DefragModuleString to ValkeyModule_DefragValkeyModuleString #242

Closed

PingXie deleted the redis_compat branch May 16, 2024 20:05

		@@ -33,8 +33,8 @@
		* The comments in this file are used to generate the API documentation on the

Uh oh!

Conversation

PingXie commented Apr 4, 2024

Uh oh!

PingXie commented Apr 4, 2024

Uh oh!

natoscott commented Apr 4, 2024

Uh oh!

zuiderkwast commented Apr 4, 2024

Uh oh!

madolson commented Apr 4, 2024

Uh oh!

Uh oh!

Uh oh!

PingXie commented Apr 4, 2024

Uh oh!

madolson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

madolson Apr 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PingXie commented Apr 5, 2024

Uh oh!

PingXie commented Apr 5, 2024

Uh oh!

zuiderkwast commented Apr 5, 2024

Uh oh!

zuiderkwast commented Apr 5, 2024

Uh oh!

madolson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

madolson Apr 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zuiderkwast commented Apr 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

madolson commented Apr 5, 2024

Uh oh!

PingXie commented Apr 5, 2024

Uh oh!

PingXie commented Apr 5, 2024

Uh oh!

zuiderkwast commented Apr 5, 2024

Uh oh!

madolson commented Apr 5, 2024

Uh oh!

PingXie commented Apr 5, 2024

Uh oh!

madolson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

madolson Apr 5, 2024 •

edited

Loading

madolson Apr 5, 2024 •

edited

Loading

zuiderkwast commented Apr 5, 2024 •

edited

Loading