-
Notifications
You must be signed in to change notification settings - Fork 21
impl: new backend API #173
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
impl: new backend API #173
Conversation
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.
Pull Request Overview
This PR refactors the KVS backend architecture to use dependency injection, making the backend implementation pluggable rather than hardcoded. The JsonBackend is now a public, configurable component with its own builder pattern.
Key changes:
- Introduced
KvsBackendtrait with methods for loading, saving, and managing snapshots - Made
JsonBackendpublic withJsonBackendBuilderfor configuration - Removed file path methods from KVS API (
get_kvs_filename,get_hash_filename) - Updated all tests and examples to use the new builder-based backend configuration
Reviewed Changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/rust/rust_kvs/src/lib.rs | Exposed backend modules and updated prelude exports |
| src/rust/rust_kvs/src/kvs_backend.rs | Defined KvsBackend trait with DynEq for dynamic equality checks |
| src/rust/rust_kvs/src/json_backend.rs | Made JsonBackend public, added builder, moved snapshot rotation logic |
| src/rust/rust_kvs/src/kvs_builder.rs | Refactored to accept backend via DI, simplified parameter handling |
| src/rust/rust_kvs/src/kvs.rs | Delegated backend operations to injected backend implementation |
| src/rust/rust_kvs/src/kvs_api.rs | Removed file path retrieval methods, added Eq derives |
| src/rust/rust_kvs/src/kvs_mock.rs | Removed mock implementations of deleted file path methods |
| src/rust/rust_kvs_tool/src/kvs_tool.rs | Added backend downcasting to access backend-specific methods |
| tests/rust_test_scenarios/src/helpers/mod.rs | Added helper for computing expected file paths |
| tests/rust_test_scenarios/src/helpers/kvs_instance.rs | Updated to build backend separately before passing to builder |
| tests/rust_test_scenarios/src/test_basic.rs | Simplified to use helper function |
| tests/rust_test_scenarios/src/cit/*.rs | Updated to use new helper and check file existence directly |
| tests/python_test_cases/tests/test_cit_snapshots.py | Updated assertions to check path strings and existence flags |
| src/rust/rust_kvs/examples/*.rs | Updated all examples to use new backend builder pattern |
| docs/class_diagram.puml | Added PlantUML class diagram documenting new architecture |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
License Check Results🚀 The license check job ran with the Bazel command: bazel run //:license-checkStatus: Click to expand output |
|
The created documentation from the pull request is available at: docu-html |
697f11d to
8db77a1
Compare
bf005a3 to
2b512a2
Compare
ad5cae6 to
e3945d4
Compare
|
please link issue in PR description. |
e3945d4 to
02ae015
Compare
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.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pawelrutkaq
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.
lgtm, since req are not ready yet code sound plausible. lets just fix doc copilot issues
02ae015 to
de5398e
Compare
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.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Backend implementation provided by DI. - Built-in `JsonBackend` is now public. - Added `JsonBackendBuilder`. - Added `docs/class_diagram.puml` with current design. - Updated tests and examples.
de5398e to
31b1397
Compare
JsonBackendis now public.JsonBackendBuilder.docs/class_diagram.pumlwith current design.Resolves #203