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

Future-Proof ValkeyModuleScriptingEngineMemoryInfo by Adding Version #1468

Open
PingXie opened this issue Dec 22, 2024 · 3 comments
Open

Future-Proof ValkeyModuleScriptingEngineMemoryInfo by Adding Version #1468

PingXie opened this issue Dec 22, 2024 · 3 comments

Comments

@PingXie
Copy link
Member

PingXie commented Dec 22, 2024

          this info struct needs to be versioned. see `ValkeyModuleClientInfo`.

Originally posted by @PingXie in #1277 (comment)

@PingXie
Copy link
Member Author

PingXie commented Dec 22, 2024

FYI @rjd15372

@zuiderkwast
Copy link
Contributor

I don't understand the discussion in the original thread. Why isn't it enough to have a version in the main entry-point struct?

@PingXie
Copy link
Member Author

PingXie commented Jan 5, 2025

sometimes we might just need a new field to the struct. bumping the ABI version would be more disruptive and might be breaking. If we version the struct, we can just add the new field to the end of the struct. The module who only understands V1 struct would still be able to parse the return info (all the way to the end of the V1 field list).

Yes, and we already version the struct ValkeyModuleScriptingEngineMethodsV1, which is the entry point for all scripting engine structs. If we add a field in any of these structs, we bump the version in the main struct ValkeyModuleScriptingEngineMethods.
@PingXie is there any benefit is versioning every single struct? And what do you mean by bumping the ABI version would be more disruptive? All we're talking about here is just versioned structs...

ValkeyModuleScriptingEngineMethodsV1 is too big a scope IMO. The use case I am considering is when we need to expose a new field in ValkeyModuleScriptingEngineMemoryInfo like below.

 typedef struct ValkeyModuleScriptingEngineMemoryInfo {
     /* The memory used by the scripting engine runtime. */
     size_t used_memory;
     /* The memory used by the scripting engine data structures. */
     size_t engine_memory_overhead;
     /* new field */
     size_t some_other_size;
 } ValkeyModuleScriptingEngineMemoryInfo;

If we had a version field in ValkeyModuleScriptingEngineMemoryInfo, we could just bump its version while maintaining compatibility with ValkeyModuleScriptingEngineMethodsV1. This seems like the idea of ValkeyModuleClientInfo too?

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

No branches or pull requests

2 participants