Skip to content
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

Add Redis cache backend - System::Cache::Redis. #112

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

Conversation

ymyasoedov
Copy link

Proposed change

This PR adds a new cache backend based on Redis.

Additional information

Checklist

  • The code change is tested and works locally.(❗)
  • There is no commented out code in this PR.(❕)
  • You improved or added new unit tests.(❕)
  • Local ZnunyCodePolicy run passes successfully.(❕)
  • Local unit tests pass.(❕)
  • GitHub workflow ZnunyCodePolicy passes.(❗)
  • GitHub workflow unit tests pass.(❗)

@rkaldung
Copy link
Member

👍

@dennykorsukewitz dennykorsukewitz self-assigned this Aug 20, 2021
@dennykorsukewitz dennykorsukewitz added 1 - 🚀 feature New feature or request 4 - clarification The issue or pull requests needs more information. 3 - wait for contributor Contributor, it's your turn. 3 - wait for reviewer Znuny, it's your turn. Znuny Feature 2 - System System/** labels Aug 20, 2021
@dennykorsukewitz dennykorsukewitz changed the title Add Redis cache backend Add Redis cache backend - System::Cache::Redis. Aug 20, 2021
@dennykorsukewitz
Copy link
Member

dennykorsukewitz commented Aug 20, 2021

Hi @ymyasoedov,

cool that you took the time and built a new cache backend. 🚀
We appreciate that very much.

So... that we can use the backend in the future, you have to complete a few tasks.

  • Create a System/Redis.pm (like following).
    We need a wrapper for all Redis functions to distinguish between OTRS and Redis.
    # examples:
    System/JSON.pm
    System/Storable.pm
    System/VirtualFS.pm
    System/XML.pm
    System/YAML.pm
    # Redis function you used.
    $Self->{Redis}->del();
    $Self->{Redis}->flushdb();
    $Self->{Redis}->get();
    $Self->{Redis}->sadd();
    $Self->{Redis}->scan();
    $Self->{Redis}->select();
    $Self->{Redis}->set();
    $Self->{Redis}->smembers();
  • Change System/Cache/Redis.pm according to the wrapper.
  • Create a UnitTest scripts/test/Redis.t for System/Redis.pm.
  • Create a UnitTest scripts/test/Cache/Redis.t for System/Cache/Redis.pm.
  • Use ZnunyCodePolicy to optimize code / readability.
  • What is the difference between RedisFast and Redis? RedisFast sounds better already.

If you have any questions or need assistance, let me know.
We and the community will help.

best regards 👍🏼

@dennykorsukewitz dennykorsukewitz removed 4 - clarification The issue or pull requests needs more information. 3 - wait for reviewer Znuny, it's your turn. labels Aug 20, 2021
@dennykorsukewitz dennykorsukewitz removed their request for review August 20, 2021 16:25
@dennykorsukewitz dennykorsukewitz added the 4 - verified This issue or pull request was verified. label Aug 25, 2021
@bschmalhofer
Copy link

Hi,

this patch adds Redis support via the Perl modules Redis or Redis::Fast. Another idea is to provide generic support for CHI, https://metacpan.org/pod/CHI, and then use CHI::Driver::Redis. This would allow easy integration of other caching modules.

@rkaldung
Copy link
Member

rkaldung commented Sep 1, 2021

Hi,

this patch adds Redis support via the Perl modules Redis or Redis::Fast. Another idea is to provide generic support for CHI, https://metacpan.org/pod/CHI, and then use CHI::Driver::Redis. This would allow easy integration of other caching modules.

@bschmalhofer Nice idea to have a generic caching layer. Do you have any experience regarding performance? As far as I see it's another layer we add when using CHI.

@bschmalhofer
Copy link

@bschmalhofer Nice idea to have a generic caching layer. Do you have any experience regarding performance? As far as I see it's another layer we add when using CHI.

I haven't used CHI myself, but it sound like the normal thing to to. I took a quick look at the code of CHI::Driver::Redis and as expected it looks like a simple wrapper of the module Redis. My guess is that the performance hit would not be noticable. We are considering CHI for OTOBO, but of course this change has no high priority, RotherOSS/otobo#107.

@dennykorsukewitz
Copy link
Member

Hi @ymyasoedov

what's the status here?
Do you have any questions or need help?

best 🚀

@ymyasoedov
Copy link
Author

Hi!

Unfortunately, I haven't enough free time for that activity. I've proposed my module as it is. Beside that, I don't see any valuable benefit of making that additional abstraction layer with reduced API compared to original Redis (or Redis::Fast) module, it won't add neither additional functionality nor convenience. Redis has its own good set of unit tests, and it looks like an unneeded duplication of work.

@ymyasoedov ymyasoedov removed their assignment Sep 9, 2022
@dennykorsukewitz
Copy link
Member

Hi

This branch has conflicts that must be resolved.

Thank you! 🚀

@dignin dignin added the 4 - help appreciated Hey, we are looking for someone to help us here. label May 17, 2023
@dignin
Copy link

dignin commented May 17, 2023

We will keep this PR open and hope for help from another developer to assist the contributor to meet the requirements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 - 🚀 feature New feature or request 2 - System System/** 3 - wait for contributor Contributor, it's your turn. 4 - help appreciated Hey, we are looking for someone to help us here. 4 - verified This issue or pull request was verified.
Development

Successfully merging this pull request may close these issues.

5 participants