-
Notifications
You must be signed in to change notification settings - Fork 486
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove REDISMODULE_ prefixes and introduce compatibility header #194
Conversation
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>
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. |
+1 for VALKEYMODULE_ prefix. |
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. |
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. |
SGTM. Will align to "valkeymodule" |
Signed-off-by: Ping Xie <pingxie@google.com>
Signed-off-by: Ping Xie <pingxie@google.com>
Signed-off-by: Ping Xie <pingxie@google.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
src/valkeymodule.h
Outdated
/* | ||
* valkeymodule.h | ||
* | ||
* This header file is renamed from valkeymodule.h to reflect the new naming conventions adopted in Valkey. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like an outdated comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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>
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.
|
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. |
No concern with that. It's more safe to postpone it actually. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -33,8 +33,8 @@ | |||
* The comments in this file are used to generate the API documentation on the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
load and unload hooks are updated. Others are indeed just find/replace. Maybe I should run clang-format on module.c now? :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eh, we're completely destroying git history, we can do it later and destroy it again.
|
||
#endif /* REDISMODULE_CORE */ | ||
#endif /* REDISMODULE_H */ | ||
#include "valkeymodule.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a big deal IMO. Users can copy it from redis 7.2.4 instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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...) |
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. |
I will take a quick look into the unload. I have also narrowed the memory leak issue to a minimum repro. |
Yeah, but even if we do introduce a better method, some modules will still use |
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>
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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just reviewed the diff, and LGTM
* 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what to what? Fixme
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
…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>
Fix #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.