-
Notifications
You must be signed in to change notification settings - Fork 160
feat(data): introduce DataApi trait and enhance DataStore functionality #13059
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
base: jans-cedarling-12349
Are you sure you want to change the base?
Conversation
- Added `DataApi` trait to define a consistent interface for data store operations, including methods for pushing, retrieving, and managing data. - Implemented `DataStoreStats` struct to provide insights into the data store's state and usage. - Updated `DataStore` to implement the `DataApi` trait, enhancing its functionality with new methods for data management. - Enhanced documentation for the data module, including examples and descriptions of new components. Signed-off-by: haileyesus2433 <haileyesusbe@gmail.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThe PR introduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
@coderabbitai please review |
|
✅ Actions performedReview triggered.
|
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.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@jans-cedarling/cedarling/src/data/store.rs`:
- Around line 878-1050: Replace direct assert_eq comparisons of
serde_json::Value or Option<Value> in the tests with test_utils::assert_eq to
get improved diffs; specifically update assertions in test_data_api_push_and_get
(the comparison of result to Some(json!({...})) and the missing Option),
test_data_api_get_entry (entry.value comparison to json!("test_value")), and any
other assertions comparing Value/Option<Value> in these tests to use
test_utils::assert_eq instead of assert_eq, keeping the same expected values and
operands but invoking test_utils::assert_eq for better output.
- Around line 264-268: list_entries currently returns every DataEntry from
storage.iter() without considering TTL; update the implementation in
list_entries (cedarling::data::store::list_entries) to filter out expired items
before collecting, e.g., only collect entries where the entry is not expired by
using the existing expiration check (call entry.is_expired() if present) or
compare the entry's expiration timestamp/ttl field against the current time, so
read paths honor the same auto_clear_expired semantics as write paths.
Signed-off-by: haileyesus2433 <haileyesusbe@gmail.com>
| /// | ||
| /// # Examples | ||
| /// | ||
| /// ```ignore |
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.
We shouldn't mark this doc-code as ignored. It should compile.
If this trait is public in example, it should use Cedarling but not the internal implementation DataStore, which will be private.
You may add implementation to Cedarling in cedarling/src/lib.rs as it was made for LogStorage.
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.
And if DataApi is supposed to be public, it should be reimported as public in cedarling/src/lib.rs
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.
resolved in 653c1b7
| //! | ||
| //! # Example | ||
| //! | ||
| //! ```ignore |
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.
We already discussed that better to avoid ignore, and if it is not possible, we need to remove that doc-code. Because it is hard to support without automatic checking if it is compiling.
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.
resolved in 653c1b7
…methods - Added `DataStore` as a member of the `Cedarling` struct to manage data storage. - Implemented the `DataApi` trait for `Cedarling`, providing methods for data manipulation including push, get, remove, and list operations. - Enhanced initialization of `Cedarling` to include a default `DataStoreConfig`. Signed-off-by: haileyesus2433 <haileyesusbe@gmail.com>
|
|
||
| // implements DataApi for Cedarling | ||
| // provides public interface for pushing and retrieving data | ||
| impl DataApi for Cedarling { |
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.
Why does the DataStore implement DataApi when Cedarling is already implementing it why not call the methods in DataStore directly here instead? Something like self.data.push(key, value, ttl) I may have missed something though
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.
no u didn't miss anything ur right, am currently refactoring it because i also have to update the tests thats why i will push the fix in a moment but thanks for pointing it out
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.
resolved in 9634d72
- Updated `Cedarling` to use direct method calls from `DataStore`, simplifying the implementation of the `DataApi` trait. - Refactored `DataStore` methods to improve clarity and maintainability, including making `list_entries` public. - Enhanced `get_stats` method to directly return statistics from the `DataStore` configuration. Signed-off-by: haileyesus2433 <haileyesusbe@gmail.com>
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.
LGTM
|
|
||
| // Initialize data store with default configuration | ||
| // TODO: Add DataStoreConfig to BootstrapConfig | ||
| let data = Arc::new( |
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.
I think eventually we will move builing DataStore to the service factory
|
I also think that the folder name |
ok i will update it |
Prepare
Description
introduce DataApi trait and enhance DataStore functionality
Target issue
closes #13043
Implementation Details
DataApitrait to define a consistent interface for data store operations, including methods for pushing, retrieving, and managing data.DataStoreStatsstruct to provide insights into the data store's state and usage.DataStoreto implement theDataApitrait, enhancing its functionality with new methods for data management.Test and Document the changes
Please check the below before submitting your PR. The PR will not be merged if there are no commits that start with
docs:to indicate documentation changes or if the below checklist is not selected.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.