Skip to content
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

Add valkeymodule.h, ABI compatible with redismodule #146

Closed
Tracked by #25
zuiderkwast opened this issue Apr 2, 2024 · 7 comments · Fixed by #194
Closed
Tracked by #25

Add valkeymodule.h, ABI compatible with redismodule #146

zuiderkwast opened this issue Apr 2, 2024 · 7 comments · Fixed by #194
Assignees

Comments

@zuiderkwast
Copy link
Contributor

zuiderkwast commented Apr 2, 2024

  • Keep the filename redismodule.h with the existing names like REDISMODULE_*, RedisModuleString, RedisModule_CreateString().
  • Add another file valkeymodule.h with the same API just everything renamed to Valkey. Make sure it is ABI compatible. (Test it in some way.)
@hpatro
Copy link
Contributor

hpatro commented Apr 4, 2024

Should we move the test modules to operate on valkey namespace API(s) ?

@madolson
Copy link
Member

madolson commented Apr 4, 2024

I think it might be good to keep them there for compatibility testing. I wonder if there is an easy way to generate modules for both.

@PingXie
Copy link
Member

PingXie commented Apr 4, 2024

I think tests can be a late binding decision. We get the coverage for both the REDISMODULE_ and VALKEY_MODULE namespaces this way.

@hpatro
Copy link
Contributor

hpatro commented Apr 4, 2024

I think tests can be a late binding decision. We get the coverage for both the REDISMODULE_ and VALKEY_MODULE namespaces this way.

@PingXie I was asking specifically for @zuiderkwast 's Test it in some way thing. Having duplicate test modules could also provide the basic testing.

Add another file valkeymodule.h with the same API just everything renamed to Valkey. Make sure it is ABI compatible. (Test it in some way.)

@PingXie
Copy link
Member

PingXie commented Apr 4, 2024

I think testing REDISMODULE_ provides the superset coverage because how the redirection is done

@madolson
Copy link
Member

madolson commented Apr 4, 2024

I think testing REDISMODULE_ provides the superset coverage because how the redirection is done

I agree. I would like to add tests for the VALKEYMODULE_, but it honestly bothers me a bit less than the lua ones. We did find a bug with adding tests for LUA, but I would be okay omitting it for now to prioritize getting a release somewhat finalized this week.

PingXie added a commit that referenced this issue Apr 5, 2024
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.

---------

Signed-off-by: Ping Xie <pingxie@google.com>
madolson pushed a commit to madolson/valkey that referenced this issue Apr 7, 2024
…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>
PatrickJS pushed a commit to PatrickJS/placeholderkv that referenced this issue Apr 24, 2024
…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>
@zuiderkwast
Copy link
Contributor Author

There's one macro we missed when renaming:

#define RMAPI_FUNC_SUPPORTED(func) (func != NULL)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging a pull request may close this issue.

4 participants