-
Notifications
You must be signed in to change notification settings - Fork 24.4k
modules API: Support register unprefixed config parameters #13656
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
Conversation
|
The interface LGTM. i don't have time to review the code at the moment. correct me if i'm wrong, but in the past, one could provide configs for module at startup, and load the module later, and the module will get the configs. or more importantly, one could config a module, then unload it, and re-load it, and it'll get the configs back. i don't think we should change that behavior, and i'd suggest that unprefixed configs will only be suitable for modules loaded at startup and never unloaded. is that different than what you implemented? let's have a discussion about that and make sure to document it. |
MeirShpilraien
left a 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.
API and code looks good 👍
Lets just address the open questions.
@oranagra, once startup done, the latest will fail if there are unmapped config, even if they have module directive. Please consider the following and let me know what you prefer to do about it: LATEST: For “aaa bbb”, with or without --loadmodule: For “aaa.bbb ccc”, with or without --loadmodule: PR: For “aaa bbb” or “aaa.bbb ccc”, with or without --loadmodule: |
|
by "LATEST" you mean "unstable", or "7.4", right? |
@oranagra , unstable & 7.4 |
oranagra
left a 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.
Approving the API and top comment.
PR redis#10285 introduced support for modules to register four types of configurations — Bool, Numeric, String, and Enum. Accessible through the Redis config file and the CONFIG command. With this PR, it will be possible to register configuration parameters without automatically prefixing the parameter names. This provides greater flexibility in configuration naming, enabling, for instance, both `bf-initial-size` or `initial-size` to be defined in the module without automatically prefixing with `<MODULE-NAME>.`. In addition it will also be possible to create a single additional alias via the same API. This brings us another step closer to integrate modules into redis core. **Example:** Register a configuration parameter `bf-initial-size` with an alias `initial-size` without the automatic module name prefix, set with new `REDISMODULE_CONFIG_UNPREFIXED` flag: ``` RedisModule_RegisterBoolConfig(ctx, "bf-initial-size|initial-size", default_val, optflags | REDISMODULE_CONFIG_UNPREFIXED, getfn, setfn, applyfn, privdata); ``` # API changes Related functions that now support unprefixed configuration flag (`REDISMODULE_CONFIG_UNPREFIXED`) along with optional alias: ``` RedisModule_RegisterBoolConfig RedisModule_RegisterEnumConfig RedisModule_RegisterNumericConfig RedisModule_RegisterStringConfig ``` # Implementation Details: `config.c`: On load server configuration, at function `loadServerConfigFromString()`, it collects all unknown configurations into `module_configs_queue` dictionary. These may include valid module configurations or invalid ones. They will be validated later by `loadModuleConfigs()` against the configurations declared by the loaded module(s). `Module.c:` The `ModuleConfig` structure has been modified to store now: (1) Full configuration name (2) Alias (3) Unprefixed flag status - ensuring that configurations retain their original registration format when triggered in notifications. Added error printout: This change introduces an error printout for unresolved configurations, detailing each unresolved parameter detected during startup. The last line in the output existed prior to this change and has been retained to systems relies on it: ``` 595011:M 18 Nov 2024 08:26:23.616 # Unresolved Configuration(s) Detected: 595011:M 18 Nov 2024 08:26:23.616 # >>> 'bf-initiel-size 8' 595011:M 18 Nov 2024 08:26:23.616 # >>> 'search-sizex 32' 595011:M 18 Nov 2024 08:26:23.616 # Module Configuration detected without loadmodule directive or no ApplyConfig call: aborting ``` # Backward Compatibility: Existing modules will function without modification, as the new functionality only applies if REDISMODULE_CONFIG_UNPREFIXED is explicitly set. # Module vs. Core API Conflict Behavior The new API allows to modules loading duplication of same configuration name or same configuration alias, just like redis core configuration allows (i.e. the users sets two configs with a different value, but these two configs are actually the same one). Unlike redis core, given a name and its alias, it doesn't allow have both configuration on load. To implement it, it is required to modify DS `module_configs_queue` to reflect the order of their loading and later on, during `loadModuleConfigs()`, resolve pairs of names and aliases and which one is the last one to apply. "Relaxing" this limitation can be deferred to a future update if necessary, but for now, we error in this case.
PR #10285 introduced support for modules to register four types of configurations — Bool, Numeric, String, and Enum. Accessible through the Redis config file and the CONFIG command.
With this PR, it will be possible to register configuration parameters without automatically prefixing the parameter names. This provides greater flexibility in configuration naming, enabling, for instance, both
bf-initial-sizeorinitial-sizeto be defined in the module without automatically prefixing with<MODULE-NAME>.. In addition it will also be possible to create a single additional alias via the same API. This brings us another step closer to integrate modules into redis core.Example: Register a configuration parameter
bf-initial-sizewith an aliasinitial-sizewithout the automatic module name prefix, set with newREDISMODULE_CONFIG_UNPREFIXEDflag:API changes
Related functions that now support unprefixed configuration flag (
REDISMODULE_CONFIG_UNPREFIXED) along with optional alias:Implementation Details:
config.c: On load server configuration, at functionloadServerConfigFromString(), it collects all unknown configurations intomodule_configs_queuedictionary. These may include valid module configurations or invalid ones. They will be validated later byloadModuleConfigs()against the configurations declared by the loaded module(s).Module.c:TheModuleConfigstructure has been modified to store now: (1) Full configuration name (2) Alias (3) Unprefixed flag status - ensuring that configurations retain their original registration format when triggered in notifications.Added error printout:
This change introduces an error printout for unresolved configurations, detailing each unresolved parameter detected during startup. The last line in the output existed prior to this change and has been retained to systems relies on it:
Backward Compatibility:
Existing modules will function without modification, as the new functionality only applies if REDISMODULE_CONFIG_UNPREFIXED is explicitly set.
Module vs. Core API Conflict Behavior
The new API allows to modules loading duplication of same configuration name or same configuration alias, just like redis core configuration allows (i.e. the users sets two configs with a different value, but these two configs are actually the same one). Unlike redis core, given a name and its alias, it doesn't allow have both configuration on load. To implement it, it is required to modify DS
module_configs_queueto reflect the order of their loading and later on, duringloadModuleConfigs(), resolve pairs of names and aliases and which one is the last one to apply. "Relaxing" this limitation can be deferred to a future update if necessary, but for now, we error in this case.