-
Notifications
You must be signed in to change notification settings - Fork 8
Implement Redis cache with unit tests #785
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?
Conversation
0fc6a74 to
583380d
Compare
ohhmm
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.
Use boost::redis
583380d to
4fa01f5
Compare
|
|
||
| bool Set(const std::string& key, const std::string& value) override; | ||
| bool Get(const std::string& key, std::string& value) override; |
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.
API Mismatch: The RedisCache implementation doesn't match the CacheBase interface. The base class requires:
virtual std::string GetOne(const std::string_view& key) = 0;
virtual bool Set(const std::string_view& key, const std::string_view& v) = 0;
virtual bool Clear(const std::string_view& key) = 0;But RedisCache implements:
bool Set(const std::string& key, const std::string& value) override;
bool Get(const std::string& key, std::string& value) override;
bool Clear() override;Needs to be updated to match the base class interface with proper parameter types (std::string_view) and method signatures.
| @@ -0,0 +1,25 @@ | |||
| cmake_minimum_required(VERSION 3.15) | |||
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.
Namespace and Directory Structure Issue: The PR adds files to omnn/rt/storage/ but the existing CacheBase is in omnn/storage/. This creates inconsistency in the codebase structure. Either:
- Move the RedisCache implementation to
omnn/storage/to match existing cache implementations, or - Move all cache implementations to
omnn/rt/storage/for consistency
Also, the include path in RedisCache.h (#include "CacheBase.h") won't work since it's looking in the wrong directory.
|
|
||
| #include <string> | ||
| #include <memory> | ||
| #include <hiredis/hiredis.h> |
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.
PR Description Suggestion: The PR description mentions "Consider boost::redis" but the implementation uses hiredis instead. The boost::redis library might be a better fit for this codebase since:
- It's more C++ idiomatic with modern C++ features
- The project already uses Boost extensively
- It would eliminate the need for manual connection management and C-style API calls
Consider evaluating boost::redis as mentioned in the PR description for a more integrated solution.
42ce40a to
3aa7f8f
Compare
727dc32 to
08e6077
Compare
98170f0 to
d6f0b2c
Compare
89902f4 to
cc04c6f
Compare
Consider boost::redis