search

DmitryTsepelev / graphql-ruby-persisted_queries

Persisted queries for graphql-ruby
MIT License
172 stars 20 forks source link

Add support for memcached store adapter #26

Closed JanStevens closed 4 years ago

JanStevens commented 4 years ago

Implements #6

I took over the pattern from Redis and adapted it for memcached using the Dalli gem. I do allow to pass in the compress and pool_size options from the Dalli gem.

Potentially you could do a lot more when using the memcached_url since it can be an array and you can add weights to your memcached instances.

Let me know what to adapt, probably a bunch of documentation needs to be added

JanStevens commented 4 years ago

Hmm I'm wondering if I should just use **args and pass them to the client so you can configure whatever you want? For redis we could do the same (Except for the connection info obviously)

DmitryTsepelev commented 4 years ago

Sorry for being silent, there was a long weekend here in Russia 😔

Hmm I'm wondering if I should just use **args and pass them to the client so you can configure whatever you want? For redis we could do the same (Except for the connection info obviously)

From the one hand—this approach is more flexible, but from the another one—we need to make sure that the message will be clear and actionable in case of error (e.g., now we check that memcached_url is not passed along with memcached_port, I wonder what the error message will be if we just pass all options to the Dalli::Client)

Do you know any important features that cannot be configured now but can be configured using the **args approach?

JanStevens commented 4 years ago

Sorry for being silent, there was a long weekend here in Russia 😔

Hmm I'm wondering if I should just use **args and pass them to the client so you can configure whatever you want? For redis we could do the same (Except for the connection info obviously)

From the one hand—this approach is more flexible, but from the another one—we need to make sure that the message will be clear and actionable in case of error (e.g., now we check that memcached_url is not passed along with memcached_port, I wonder what the error message will be if we just pass all options to the Dalli::Client)

Do you know any important features that cannot be configured now but can be configured using the **args approach?

I think it's still a good idea to parse the connection properties as we do now, just the additional properties like pool_size and compress could be send directly to the client.

It makes this gem a bit more flexible and resilient to change, if for example dalli adds a super awesome new functionality everyone wants the using **args you don't have to do anything to support it.

I think we can safely assume that the underlying client (redis or dalli) will have enough "validation" and "logic" to prevent wrong attribute usage (in most cases it probably ignore invalid options)

DmitryTsepelev commented 4 years ago

Agreed, let's try it out!

JanStevens commented 4 years ago

Agreed, let's try it out!

Great ill cook something up

JanStevens commented 4 years ago

LGTM! Should I merge it or you plan to make some final touches?

Nope should be good to go, I can do the redis one aswell but that will be a different MR

DmitryTsepelev commented 4 years ago

Cool cool, thanks again!