-
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
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