search

3scale / apisonator

Red Hat 3scale API Management Apisonator backend
https://3scale.net
Apache License 2.0
36 stars 27 forks source link

THREESCALE-11061: Async mode: Add support for Redis logical DBs #390

Closed jlledom closed 1 month ago

jlledom commented 1 month ago

This comes from of https://github.com/3scale/apisonator/pull/350.

Now the Async mode supports Redis logical databases, and honors URLs like redis://localhost:6379/6

The support has been also added for sentinels. A typical config would look like this:

CONFIG_REDIS_PROXY=redis://redis-master/6
CONFIG_REDIS_SENTINEL_HOSTS=redis://localhost:26379,redis://localhost:26380,redis://localhost:26381

Jira: https://issues.redhat.com/browse/THREESCALE-11061

jlledom commented 1 month ago

From https://github.com/3scale/apisonator/pull/389#issuecomment-2121163703:

@akostadinov

Instead of monkey patching upstream protocol class, it is a much better approach to use our own class and use the upstream protocol class with the composition pattern. Like I commented here: #350 (comment)

I don't understand what you mean. I'm not monkey patching the original protocol class, I added a new class ExtendedRESP2 which in fact is pretty similar to the example you mentioned:

https://github.com/socketry/async-redis/blob/feb67542dc27947fddbc647ad4b7289645975698/examples/auth/wrapper.rb#L52-L74

akostadinov commented 1 month ago

I added a new class ExtendedRESP2 which in fact is pretty similar to the example you mentioned

Because I saw you using the Async namespace, I thought this is monkey-patching the class. Could you use our own namespace for the class? It's clearer what comes from where.

jlledom commented 1 month ago

I added a new class ExtendedRESP2 which in fact is pretty similar to the example you mentioned

Because I saw you using the Async namespace, I thought this is monkey-patching the class. Could you use our own namespace for the class? It's clearer what comes from where.

True. Fixed in https://github.com/3scale/apisonator/pull/390/commits/41a9925738ecd5eff28879ef56861461edd64ae4

jlledom commented 1 month ago

Nice! It's a bummer that Redis is somehow conflicting though. I'm fine to keep it as is. One possible option is to call the module AsyncRedis for example. But I leave it to your preference.

Yeah, I think I'm gonna rename it.

jlledom commented 1 month ago

Nice! It's a bummer that Redis is somehow conflicting though. I'm fine to keep it as is. One possible option is to call the module AsyncRedis for example. But I leave it to your preference.

Yeah, I think I'm gonna rename it.

@akostadinov done in https://github.com/3scale/apisonator/pull/390/commits/7a2da06b635818bfda7d0401b7357d322e177655