The PR introduce small API fixes/changes. #302
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Main changes: * Key space notification API will get the keys as `&[u8]` instead of `&str` because key name can be binary. * ErrorReply was changed to be an enum of `RedisError` or general message so we can return this error type even without having an actual `RedisModuleCallReply` error. * Introduce `DetachContext` struct which inplements `Sync` and `Send`. This can be used to create a global context for logging only. * `replicate` can accept also binary data and not only utf8. * `create_string` can accept binary data and not only utf8. * `autenticate_user` implementation was fixed to use the correct `RedisModuleAPI`. * `ThreadSafeContext` lock function was fixed to avoid reuse the context and avoid duplicate context free. * `RedisModule_Init` was split to `RedisModule_Init` and `RedisModule_InitAPI` so we can intialize the API without register a module. This is usefull for modules that load more plugins and want to intialize the `RedisModuleAPI` on the plugin but without Register it as a another module. We should consider backport this change to Redis. * Move `init_func` callback after finish registration of commands and configuration so configuration value will be applied when called. * Introduce `safe_clone` for `RedisModuleString`. In general `RedisModuleString` is none atomic ref counted object. So it is not safe to clone it if Redis GIL is not hold. `safe_clone` gets a context reference which indicating that Redis GIL is held. * Implement serde serialize and deserialize for `RedisString`
MeirShpilraien
force-pushed
the
ap_fixes_changes
branch
from
bf8d44b to
f3661da
Compare
iddm
reviewed
Apr 3, 2023
This was referenced Apr 3, 2023
|
Thanks @vityafx I believe I addressed all comments. Please take another look to make sure I did not miss anything. |
iddm
reviewed
Apr 4, 2023
iddm
reviewed
Apr 4, 2023
iddm
reviewed
Apr 4, 2023
iddm
reviewed
Apr 4, 2023
iddm
reviewed
Apr 4, 2023
|
Thanks, will wait for this one to merge as it is depends on it: #280 |
|
@vityafx the last commit are only conflicts fixes |
iddm
approved these changes
Apr 5, 2023
MeirShpilraien
added a commit
that referenced
this pull request
Apr 24, 2023
The PR is a proposal for API versioning using feature flags. The PR introduce 4 new feature flags: * `min-redis-compatibility-version-7-2` * `min-redis-compatibility-version-7-0` * `min-redis-compatibility-version-6-2` * `min-redis-compatibility-version-6-0` - default User should set one of those feature flag which will indicate the minimum Redis version he want to be compatible with. Base on this, redismodule-rs will decide which API can be used and will manipulate existing API so it can be used conditionally. For example, take the following API: ``` pub fn add_post_notification_job<F: Fn(&Context)>(self, callback: F) ``` This API require Redis version 7.2, so if `min-redis-compatibility-version-7-2` feature is set the API is exposed as is. But if some other `min-redis-compatibility-version-*` is set, the API is changed to return `APIResult` which will allow to conditionally use this API if exists on the current Redis instance. The PR also adds support for post notifications jobs and demonstrate how to use the new `min-redis-compatibility-version-*` flags. Depends on: #302
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Main changes:
&[u8]instead of&strbecause key name can be binary.RedisErroror general message so we can return this error type even without having an actualRedisModuleCallReplyerror.DetachContextstruct which implementsSyncandSend. This can be used to create a global context for logging only.replicatecan accept also binary data and not only utf8.create_stringcan accept binary data and not only utf8.authenticate_userimplementation was fixed to use the correctRedisModuleAPI.ThreadSafeContextlock function was fixed to avoid reuse the context and avoid duplicate context free.RedisModule_Initwas split toRedisModule_InitandRedisModule_InitAPIso we can initialise the API without register a module. This is usefull for modules that load more plugins and want to initialise theRedisModuleAPIon the plugin but without Register it as a another module. We should consider back-port this change to Redis.init_funccallback after finish registration of commands and configuration so configuration value will be applied when called.safe_cloneforRedisModuleString. In generalRedisModuleStringis none atomic ref counted object. So it is not safe to clone it if Redis GIL is not hold.safe_clonegets a context reference which indicating that Redis GIL is held.RedisStringmodule_args_as_configurationoption to work correctly on imutable configurations.Depends on: #280