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

Post notification API and API versioning #304

Merged
merged 14 commits into from
Apr 24, 2023

Conversation

MeirShpilraien
Copy link
Collaborator

@MeirShpilraien MeirShpilraien commented Apr 3, 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

iddm
iddm previously approved these changes Apr 4, 2023
Copy link
Collaborator

@iddm iddm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job! 👍

redismodule-rs-derive/Cargo.toml Outdated Show resolved Hide resolved
redismodule-rs-derive/src/api_versions.rs Outdated Show resolved Hide resolved
src/context/mod.rs Outdated Show resolved Hide resolved
Base automatically changed from ap_fixes_changes to master April 5, 2023 11:27
@iddm
Copy link
Collaborator

iddm commented Apr 13, 2023

So with this change, we should split the macros into two parts: internal and external. External is for the event handlers and other stuff for a user module, and internal is the stuff which only the redismodule-rs requires. I have already done that in my fork; please check it out: https://github.com/vityafx/redismodule-rs/tree/crate-refactoring-and-macros

@MeirShpilraien
Copy link
Collaborator Author

So with this change, we should split the macros into two parts: internal and external. External is for the event handlers and other stuff for a user module, and internal is the stuff which only the redismodule-rs requires. I have already done that in my fork; please check it out: https://github.com/vityafx/redismodule-rs/tree/crate-refactoring-and-macros

Yes I agree it makes sense to split it.

@MeirShpilraien MeirShpilraien marked this pull request as ready for review April 16, 2023 11:39
Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
examples/events.rs Outdated Show resolved Hide resolved
redismodule-rs-derive-internals/Cargo.toml Outdated Show resolved Hide resolved
redismodule-rs-derive-internals/src/api_versions.rs Outdated Show resolved Hide resolved
redismodule-rs-derive-internals/src/lib.rs Outdated Show resolved Hide resolved
redismodule-rs-derive-internals/src/lib.rs Outdated Show resolved Hide resolved
redismodule-rs-derive-internals/src/lib.rs Outdated Show resolved Hide resolved
@MeirShpilraien
Copy link
Collaborator Author

@vityafx I think I addressed all the comments other then the naming which I commented on. Let me know what you think.

iddm
iddm previously approved these changes Apr 24, 2023
redismodule-rs-macros/src/lib.rs Show resolved Hide resolved
redismodule-rs-macros/src/lib.rs Outdated Show resolved Hide resolved
@MeirShpilraien MeirShpilraien changed the title WIP: post notification API and API versioning Post notification API and API versioning Apr 24, 2023
@MeirShpilraien MeirShpilraien merged commit 308a3ef into master Apr 24, 2023
@MeirShpilraien MeirShpilraien deleted the post_notification_job_and_api_versioning branch April 24, 2023 09:25
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

Successfully merging this pull request may close these issues.

2 participants