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 Valkey support #106

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Add Valkey support #106

wants to merge 11 commits into from

Conversation

amirreza8002
Copy link
Contributor

hi
this is just a prototype, check if you're okay with the way i went, then i'll try to add valkey to the rest of the codebase

one problem with this method is that you can only have redis or valkey, and not both

@AliRn76
Copy link
Owner

AliRn76 commented Oct 29, 2024

The scenario of this implementation is ok 👍🏽

one problem with this method is that you can only have redis or valkey, and not both

That is not a problem, users can use both of them, but in the implementation of the core we gave more priority to redis than valkey (also you may put a variable in configs which users can choose to use which of them for built-in stuff)

@amirreza8002
Copy link
Contributor Author

one thought that came to my mind was to go with something close to what django does, which adds a good amount of costumizibillity, tho it's more code.
I'm on the road for a day or two and can't code, so think on it.
if you want to keep this I'll code as is,
and if you want to go django style I'll do that

@AliRn76
Copy link
Owner

AliRn76 commented Oct 29, 2024

I'm ok with the current style but I'm not sure what you mean by something close to Django and something more customizable, please give me an overview of what you have in mind.

db: DatabaseConnection = DatabaseConnection()
redis: RedisConnection = RedisConnection()
valkey: ValkeyConnection = ValkeyConnection()
Copy link
Owner

Choose a reason for hiding this comment

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

As we have so many places that use redis and we have to or them with valkey, I think it would be better to have a variable named cache which stores the correct value.
And if we go with this new style, we have to have a new abstract class for the redis and valkey to save the structure

@amirreza8002
Copy link
Contributor Author

amirreza8002 commented Oct 29, 2024

in django you set any cache backend you have in a dictionary like this: example
you can set any number of backends

then, django provides you with this two objects.
caches stores information about all the backends set in settings, and you can access any of them as you wish
cache is the main backend (named default)

it has a number of features, such as:

  1. you (or a 3rd party) can add any number of backends, for any cache server, easily.
  2. you have one object (cache) that works with any backend that was set.

and you don't have to check which server has been configured, which is gonna cause problem as this package grows
i mean if server := ... is already long, imagine if you want to add a new server

anyway, it's just a thought, i'll write this the way you prefer.

@AliRn76
Copy link
Owner

AliRn76 commented Oct 29, 2024

This is a good thought, I'll come back to you ...

@AliRn76
Copy link
Owner

AliRn76 commented Nov 1, 2024

I searched a bit and because the usage and interface of other caches are not the same enough I think it is better to implement them separately, as you did, Otherwise it needs a huge amount of code to wrap all of them.
On the other hand, the end user(developer) knows exactly what service is used for caching by looking at its name.
But we have a problem with the usage of caching in the core of Panther, I suggest doing something like this, but you may have a better solution.

@AliRn76 AliRn76 marked this pull request as ready for review November 1, 2024 16:58
@AliRn76 AliRn76 added the enhancement New feature or request label Nov 1, 2024
@amirreza8002
Copy link
Contributor Author

hi!
it might need some amount of code to implement the interface, but it would make the cache interface consistent,
which would allow other tools to use the cache system.
and also allow people to implement their own backend and plug it in.

it's a trade off.
but as a package maintainer, i understand that you want to keep the code base smaller, so I'll do that.

I'll get to work in a day or two, have to fix a broken CI first :/

db: DatabaseConnection = DatabaseConnection()
redis: RedisConnection = RedisConnection()
valkey: ValkeyConnection = ValkeyConnection()

if redis.is_connected:
Copy link
Owner

Choose a reason for hiding this comment

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

The is_connected attr only can be True after the Panther.load_configs() so this condition is not right.
You may want to use design patterns for this variable, I'm going to think about the best solution too, and I'll come back to you if I find any.

@@ -148,7 +148,7 @@ def print_info(config: Config):

# Message
info_message = f"""{logo}
{h} Redis: {rd} \t {h}
{h} Cache Server: {rd} \t {h}
Copy link
Owner

Choose a reason for hiding this comment

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

Fix the spacing between {rd} and \t

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants