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

API extentions. #229

Open
wants to merge 54 commits into
base: master
Choose a base branch
from
Open

API extentions. #229

wants to merge 54 commits into from

Conversation

MeirShpilraien
Copy link
Collaborator

@MeirShpilraien MeirShpilraien commented May 17, 2022

  1. Cursor API to scan the key space. - Added keys scan API. #265
  2. Stream API: - Added API to read and trim a stream. #266
    • Read range from streams.
    • Trim stream.
  3. Server Events API to register to the following events: - Added API to register to server events. #267
    • Flush started/ended.
    • Loading started/ended for AOF/RDB/Replication.
    • Role changed (become primary/replica).
  4. Is primary API was added to Context object. - Added API to get ctx flags. #270
  5. Reply* API, to return replies without the need to copy buffers.
  6. Key missed notification - Added keymiss event support #289
  7. Extended RM_Call that allows setting call options. - Added RM_Call options #290
  8. Update redismodule.h to v7.0.2 - cleaning clippy warnings #253
  9. Added support for context acl authentication - Adde API for ACL validation #294
  10. Added API to key acl key permission - Adde API for ACL validation #294
  11. Added API to check if server is OOM - Added API to get ctx flags. #270
  12. Configuration API base on new Redis 7 module configuration.
  13. Support for resp3 on MR_Call. - Added RM_Call options #290
  14. Support resp3 on return value from commands - add RESP3 new APIs #280

todo:

  1. test RM_CallExt
  2. test context acl authentication
  3. test acl_check_key_permissions
  4. test is_oom
  5. test configuration api
  6. test resp3 on RM_Call

@MeirShpilraien MeirShpilraien marked this pull request as draft May 17, 2022 13:56
1. Cursor API to scan the key space
2. Stream API
   * Read range from streams
   * Trim stream
3. Server Events API to register to the following events:
   * Flush started/ended
   * Loading started/ended for AOF/RDB/Replication
   * Role changed (become primary/replica)
4. Is primary API was added to Context object
@MeirShpilraien MeirShpilraien changed the title WIP - API extentions. API extentions. May 26, 2022
@MeirShpilraien MeirShpilraien marked this pull request as ready for review May 26, 2022 11:40
Copy link
Contributor

@oshadmi oshadmi left a comment

Choose a reason for hiding this comment

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

👏🏼 💪🏼 Some questions

examples/scan.rs Outdated Show resolved Hide resolved
examples/stream.rs Outdated Show resolved Hide resolved

let stream_key = args.next_arg()?;

let stream = ctx.open_key(&stream_key);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just making sure - could also test for null here but can also skip this test since it is also handled in key_type() (since the common usage would not be a missing key, it would be better to skip a null test here and let key_type() handle nulls)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, key_type should verify it.

Comment on lines +30 to +31
let num_loading = unsafe { &mut NUM_LOADINGS };
*num_loading = *num_loading + 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems equivalently unsafe as
unsafe {NUM_LOADINGS += 1};

(same with NUM_FLUSHES and NUM_ROLED_CHANGED)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, but I wanted the unsafe part to be as minimal as possible

examples/scan.rs Outdated Show resolved Hide resolved
src/context/server_events.rs Outdated Show resolved Hide resolved
&context,
crate::redis_module::context::server_events::ServerEvents::$server_event_type,
Box::new($server_event_handler))) {
context.log_warning(&format!("Failed register server event, {}", err));
Copy link
Contributor

Choose a reason for hiding this comment

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

Does err include the server_event_type so there is no need to log it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is a relevant error on subscribe_to_server_event.

RedisString::from_redis_module_string(self.ctx, field_val),
));
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we verify fields len is indeed num_fields?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I do not see a reason to, the purpose is not to test Redis, we assume Redis has his own tests for this api.

tests/integration.rs Outdated Show resolved Hide resolved
}

#[derive(Clone)]
pub enum ServerEventData {
Copy link
Contributor

Choose a reason for hiding this comment

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

There are several more events (even with oss), for example, RedisModuleEvent_ClientChange, RedisModuleEvent_Shutdown, etc.
But we only support a subset now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will add it gradually when needed.

@gkorland gkorland requested a review from oshadmi July 17, 2022 10:19
iddm and others added 12 commits February 21, 2023 13:59
Within the macro_rules! the $crate should be used when it is required to
refer to the current crate.
This commit changes the referencing to the correct one.
1. Fixes the warning of the unused result. Now the result is explicitly
   dropped and so the intention behind the code written is clear.
2. Fixes the typo - "emmutable" to "immutable".
By deriving Ord and PartialOrd we can compare two versions easier.
We need to be able to compare the versions to make sure the compatible
version is used with the module.
…bility-checking

Allow for easier version compatibility checking.
Such situations occur when, for example, the crate which uses
the redismodule-rs crate as a dependency is run for testing, so
without Redis available. In this case, there is no allocator available
as well and it will lead to a panic. To enable both the situations to
work correctly under all circumstances, this change introduces a
fallback mechanism back to the system allocator (default Rust allocator)
which is always available.

Note that the changes are zero-cost: the code before this changes used
unwrap() which also had checks for the value being non-null and would
panic at runtime if it was null, the current code does this check as
well but instead of panicking reverts back to the system allocator.
The previous code either wouldn't compile or wouldn't run due to missing
assets for the tests. This commit brings changes necessary to enable the
cargo test to run properly again.
Reverts the behaviour to panicking when the redis allocator isn't
present. Changes the panic to avoid allocations using the specified
allocator so that a meaningful message can be observed.
…allocator-when-redis-isnt-available

Allow the crate to operate when the Redis API isn't available.
The backwards compatibility is broken with the old Rust due to fixing a
mistake with imports. This commit uses the direct import for unix
systems of the AsRawFd trait and should work on all unix systems.
Fix the backwards compatibility with old Rust.
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.

4 participants