Skip to content

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ohhmm
Copy link
Owner

@ohhmm ohhmm commented Feb 19, 2025

Consider boost::redis

@ohhmm ohhmm force-pushed the devin/1732269265-implement-redis-cache branch 3 times, most recently from 0fc6a74 to 583380d Compare March 1, 2025 19:12
Copy link
Owner Author

@ohhmm ohhmm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use boost::redis

@ohhmm ohhmm force-pushed the devin/1732269265-implement-redis-cache branch from 583380d to 4fa01f5 Compare March 8, 2025 13:39
Comment on lines +17 to +19

bool Set(const std::string& key, const std::string& value) override;
bool Get(const std::string& key, std::string& value) override;
Copy link
Contributor

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)
Copy link
Contributor

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:

  1. Move the RedisCache implementation to omnn/storage/ to match existing cache implementations, or
  2. 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>
Copy link
Contributor

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:

  1. It's more C++ idiomatic with modern C++ features
  2. The project already uses Boost extensively
  3. 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.

@ohhmm ohhmm force-pushed the devin/1732269265-implement-redis-cache branch 3 times, most recently from 42ce40a to 3aa7f8f Compare March 16, 2025 09:46
@ohhmm ohhmm force-pushed the devin/1732269265-implement-redis-cache branch 5 times, most recently from 727dc32 to 08e6077 Compare March 23, 2025 18:23
@ohhmm ohhmm force-pushed the devin/1732269265-implement-redis-cache branch 2 times, most recently from 98170f0 to d6f0b2c Compare March 28, 2025 17:29
@ohhmm ohhmm force-pushed the devin/1732269265-implement-redis-cache branch 2 times, most recently from 89902f4 to cc04c6f Compare March 30, 2025 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant