[object_cache] add application id to object cache#346
[object_cache] add application id to object cache#346jinzhejz wants to merge 1 commit intoxflops:mainfrom
Conversation
Summary of ChangesHello @jinzhejz, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the object caching system by introducing an Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces application_id to the object cache keying scheme, changing it to {application_id}/{session_id}/{object_id}. This is a significant and necessary enhancement for multi-application environments. The changes are comprehensive, touching the cache server, Python SDK, documentation, and tests. The addition of a standalone binary for the object cache and extensive integration tests are particularly valuable.
My review identifies a couple of issues in the Rust implementation related to backward compatibility. The attempt to support legacy 2-part keys in do_put seems to be broken, and the handling of legacy formats in do_action is confusing. Addressing these will improve the robustness and clarity of the implementation.
| } else if parts.len() == 2 { | ||
| // Legacy format: session_id/object_id (for backward compatibility) | ||
| *session_id = Some(parts[0].to_string()); | ||
| *object_id = Some(parts[1].to_string()); | ||
| } |
There was a problem hiding this comment.
The backward compatibility logic for 2-part paths (session_id/object_id) in do_put operations appears to be incomplete. When a legacy client provides a 2-part path, application_id is not set. The subsequent logic in collect_batches_from_stream requires an application_id and will fail, as the Python client does not seem to set the app_metadata fallback.
This means the do_put operation will fail for legacy clients, despite the code suggesting backward compatibility.
To fix this, you could consider using a default application_id for legacy paths or making application_id optional in put_with_id for this specific case. If backward compatibility is not a goal for do_put, this code path should be removed to avoid confusion.
| // Legacy format: only session_id (for backward compatibility) | ||
| // Try to infer application_id from existing objects | ||
| return Err(FlameError::InvalidState( | ||
| "DELETE action requires application_id/session_id format".to_string(), | ||
| )); | ||
| }; |
There was a problem hiding this comment.
The comment on line 680 suggests backward compatibility for the legacy DELETE action format, but the implementation immediately returns an error. Furthermore, the comment on line 681, "Try to infer application_id from existing objects", describes logic that is not implemented.
This is misleading. If the intention is to break compatibility for the DELETE action, the comments should be removed to reflect the actual behavior. If backward compatibility is desired, the logic to infer application_id would need to be implemented.
582b1fa to
3498cba
Compare
No description provided.