impl: backends registry, KvsMap backend params
#139
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The created documentation from the pull request is available at: docu-html |
License Check Results🚀 The license check job ran with the Bazel command: bazel run //:license-checkStatus: Click to expand output |
arkjedrz
force-pushed
the
arkjedrz_backend-registry
branch
2 times, most recently
from
caa96d6 to
5038467
Compare
Contributor
Author
|
arkjedrz
force-pushed
the
arkjedrz_backend-registry
branch
2 times, most recently
from
bf13745 to
e34c09b
Compare
- New `KvsBackend` API - independent from filesystem.
- Make `JsonBackend` explicitly default, allow others.
- Add `JsonBackendBuilder`.
- This is required to allow for defaults override without setters.
- E.g., `snapshot_max_count` can be set, but shouldn't change after
init.
- Update tests.
arkjedrz
force-pushed
the
arkjedrz_backend-registry
branch
from
e34c09b to
f44a127
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the KVS backend architecture to support pluggable backends through a registry pattern. The key changes include:
- Introduction of
KvsBackendRegistryfor managing backend factories - Modified
KvsBuilderto accept backend parameters viaKvsMapinstead of direct method calls - Removed public API methods for retrieving KVS and hash file paths (
get_kvs_filename,get_hash_filename)
Reviewed Changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/rust/rust_kvs/src/kvs_backend_registry.rs | Introduces registry for backend factories with registration and retrieval functionality |
| src/rust/rust_kvs/src/kvs_backend.rs | Refactors backend interface to use instance-aware methods instead of static path resolution |
| src/rust/rust_kvs/src/json_backend.rs | Implements new backend interface with factory pattern and builder for JSON backend |
| src/rust/rust_kvs/src/kvs_builder.rs | Replaces direct configuration methods with backend_parameters accepting KvsMap |
| src/rust/rust_kvs/src/kvs.rs | Removes file path retrieval methods and delegates backend operations to backend instance |
| src/rust/rust_kvs/src/kvs_api.rs | Removes get_kvs_filename and get_hash_filename from public API |
| tests/rust_test_scenarios/src/helpers/kvs_instance.rs | Updates helper to construct backend parameters as KvsMap |
| src/rust/rust_kvs_tool/src/kvs_tool.rs | Removes operations for getting KVS and hash filenames, updates to use backend parameters |
| tests/python_test_cases/tests/test_cit_snapshots.py | Updates test assertions to check file existence directly instead of API calls |
| src/rust/rust_kvs/examples/*.rs | Updates all examples to use new backend parameter pattern |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| builder.dir(dir) | ||
| let backend_parameters = KvsMap::from([ | ||
| ("name".to_string(), KvsValue::String("json".to_string())), | ||
| ("name".to_string(), KvsValue::String(dir)), |
There was a problem hiding this comment.
The key "name" is inserted twice in the map, which will overwrite the first entry with value "json". The second insertion should use "working_dir" as the key instead of "name".
Suggested change
| ("name".to_string(), KvsValue::String(dir)), | |
| ("working_dir".to_string(), KvsValue::String(dir)), |
- Added `KvsBackendRegistry`. - `KvsBuilder` accepts backend params through `KvsMap`. - Built-in backend access is now hidden. - `backend_registration` example. - PlantUML-based class diagram.
arkjedrz
force-pushed
the
arkjedrz_backend-registry
branch
from
f44a127 to
5ef20f7
Compare
Contributor
Author
|
Merged changes with other PR, moved here #150 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
KvsBackendRegistry.KvsBuilderaccepts backend params throughKvsMap.