feat: add app info into object key.#425
Conversation
Signed-off-by: Klaus Ma <klausm@nvidia.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements RFE423, transitioning the object cache key structure from a two-level format to a three-level format: <app_name>/<session_id>/<object_id>. This change enables application-level object sharing across sessions, supported by a new put_object method in the Runner class and the introduction of reserved session names like 'shared'. The implementation spans the Rust cache server, disk storage logic, and the Python SDK, including a new ObjectKey utility for key parsing and validation. Review feedback highlights the need to update remaining legacy tests to the new format, refine internal error messages in the Rust implementation, and extend key component validation in the Python SDK to explicitly disallow forward slashes.
| def test_cache_put_and_get(): | ||
| """Test basic put and get operations.""" | ||
| session_id = "test-session-001" | ||
| key_prefix = "test-app/test-session-001" |
There was a problem hiding this comment.
While this test has been updated to use the new three-level key format (<app>/<ssn>), several other tests in this file (e.g., test_patch_single_delta, test_patch_multiple_deltas, etc., starting from line 98) still use the old two-level format. Since the API now strictly enforces the new format, those tests will fail. Please ensure all tests in this file are updated to use the app-prefixed key format.
| let key = format!("{}/{}", session_id, object_id); | ||
| let key = object_key | ||
| .to_key() | ||
| .ok_or_else(|| FlameError::Internal("Failed to generate object key".to_string()))?; |
There was a problem hiding this comment.
The error message "Failed to generate object key" is slightly misleading here. In this context, object_key is guaranteed to have an object_id (either provided or generated), so to_key() should not fail. If it does, it's an internal invariant violation rather than a failure to generate the key. Consider using .expect("object_id must be set") or a more descriptive internal error.
| .ok_or_else(|| FlameError::Internal("Failed to generate object key".to_string()))?; | |
| .to_key() | |
| .ok_or_else(|| FlameError::Internal("ObjectKey missing object_id in put_with_id".to_string()))?; |
| for name, value in [("app_name", self.app_name), ("session_id", self.session_id)]: | ||
| if not value: | ||
| raise ValueError(f"{name} cannot be empty") | ||
| if ".." in value or "\\" in value: |
There was a problem hiding this comment.
The validation logic should also check for the forward slash (/) character in key components. Since the slash is used as a separator in the three-level key structure, allowing it within a component (like app_name) would lead to parsing ambiguities and potential errors when the key is reconstructed or validated on the server.
| if ".." in value or "\\" in value: | |
| if ".." in value or "\\" in value or "/" in value: |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Signed-off-by: Klaus Ma <klausm@nvidia.com>
Signed-off-by: Klaus Ma <klausm@nvidia.com>
1 participant
No description provided.