-
Notifications
You must be signed in to change notification settings - Fork 18
impl: backend design change #150
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: main
Are you sure you want to change the base?
impl: backend design change #150
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 implements a significant backend design change by introducing a generic KVS backend system that separates the storage layer from the core KVS functionality. It replaces the filesystem-specific architecture with a pluggable backend system using the KvsBackend trait and KvsBackendRegistry.
- Introduces
KvsBackendtrait andKvsBackendRegistryfor pluggable storage backends - Replaces direct filesystem operations with backend abstraction layer
- Updates KVS builder to use backend parameters instead of direct filesystem paths
- Removes deprecated file path APIs and updates examples/tests to use new backend system
Reviewed Changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/rust/rust_kvs/src/lib.rs | Updates prelude exports and type aliases to support new backend architecture |
| src/rust/rust_kvs/src/kvs_backend_registry.rs | Implements new backend registry system for managing pluggable storage backends |
| src/rust/rust_kvs/src/kvs_backend.rs | Defines new KVS backend trait interface replacing filesystem-specific operations |
| src/rust/rust_kvs/src/kvs_builder.rs | Refactors builder to use backend parameters instead of direct filesystem paths |
| src/rust/rust_kvs/src/kvs.rs | Updates KVS implementation to use backend abstraction instead of direct file operations |
| src/rust/rust_kvs/src/json_backend.rs | Converts JSON backend to implement new backend trait with factory pattern |
| tests/rust_test_scenarios/src/helpers/ | Updates test helpers to use new backend parameter system |
| tests/python_test_cases/tests/test_cit_snapshots.py | Updates tests to check file existence rather than API return values |
| src/rust/rust_kvs_tool/src/kvs_tool.rs | Removes deprecated filename APIs and updates to use backend parameters |
| src/rust/rust_kvs/examples/ | Updates all examples to use new backend parameter format |
| docs/class_diagram.puml | Adds PlantUML class diagram showing new backend architecture |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or 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 |
|
|
||
| package "kvs_backend" { | ||
| +interface KvsBackend { | ||
| +load_kvs(instance_id: InstanceId, snapshot_id: SnapshotId): KvsMap |
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.
With the proposed API, the user always have to wait until whole KVS is populated only then values are accessible.
Why not to allow direct access to the value and satisfy requirements for earliest possible availability?
With restriction that only read access is possible eg. load_value(key: string): KvsValue
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.
Yes, currently it is assumed that storage is fully memory-mapped. You can add it as a topic for requirements discussion.
| +load_kvs(instance_id: InstanceId, snapshot_id: SnapshotId): KvsMap | ||
| +load_defaults(instance_id: InstanceId): KvsMap | ||
| +flush(instance_id: InstanceId, kvs_map: KvsMap) | ||
| +snapshot_count(instance_id: InstanceId): usize |
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.
Maybe I didn't get the whole idea behind the snapshots, my understanding and some questions:
- User can create snapshot in order to "conserve" the current values
- After values in working copy were modified, user can restore the "conserved" values with
snapshot_restore(instance_id: InstanceId, snapshot_id: SnapshotId) - Is the
snapshot_restore()intended to overwrite the working copy? I see that it return KvsMap so probably not. - Do we need the
snapshot_restore()at all?load_kvs()can take snapshot ID as well - Who is taking care of book-keeping the SnapshotIds? If this is the backend, then we need something like
get_snapshots()which will return list of existing snapshot IDs snapshot_delete()will be needed as well- How are the snapshots created? Do we need something like
snapshot_create()?
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.
snapshot_restoreis intended to overwrite current state, You still need to flush.- No strong opinion on whether we need
snapshot_restoreat all. It's just load with additional error checks, we can also provide it as an already implemented trait method, since I cannot imagine other behavior than current. SnapshotIdbehavior should be documented. Right now it is assumed that "0" means current, and "1", "2", "123" means 1 ago, 2 ago, 123 ago.snapshot_deleteis already required, but not implemented. This function will break theSnapshotIdbehavior explained above, we'll need to reconsider.- We don't need
snapshot_create, sinceflushstores current state as "0". Previously existing snapshots are rotated ("0"->"1", "1"->"2") or removed (ifsnapshot_max_count=3, then "2" is removed).
Those are valid points, but should be taken into consideration once requirements are modified.
- Created new `KvsBackend` API. - No longer filesystem-specific. - Added `KvsBackendRegistry`. - `KvsBuilder` accepts backend parameters using `KvsMap`. - `JsonBackend` used as default. - Built-in backend access is not hidden. - `backend_registration` example. - PlantUML-based class diagram. - Updated tests.
e595e08 to
9d08100
Compare
KvsBackendAPI.KvsBackendRegistry.KvsBuilderaccepts backend parameters usingKvsMap.JsonBackendused as default.backend_registrationexample.