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 moneta cache store #1017

Closed
wants to merge 2 commits into from
Closed

add moneta cache store #1017

wants to merge 2 commits into from

Conversation

minad
Copy link
Contributor

@minad minad commented Jan 21, 2013

Add support for moneta cache store http://rubygems.org/gems/moneta. You might even consider switching your whole cache system to Moneta.

@dariocravero
Copy link

Hey @Minda thanks for the PR!

It makes sense to rethink the caching mechanism itself and use something that actually provides a better integration and is fully dedicated to it. On the other hand, Padrino is meant to be agnostic and even though Moneta allows the users to pick from a good amount of backends it forces them to use it first adding another dependency -yes, I know, Tilt does the same with templates. Thoughts @padrino/core-members?

As for this PR itself, I think it's good to add it but can I ask you to rename @moneta for @backend to follow the conventions on the other stores? Also, how's Moneta's handling non expiring key/value pairs? On the docs I see that expires should be set to false but on your implementation you only set it to an expiry value if it was and leave the rest of the responsibility to Moneta's builder/initialization; is that its recommended approach to handle that?

@minad
Copy link
Contributor Author

minad commented Jan 21, 2013

  1. Moneta is relatively thin, so the dependency is not a real problem. It is inspired by Tilt and Rack, and should be quite reusable.
  2. If you don't provide the expires option, the default expires value will be used (if you don't provide one, infinity) You can set the expires option to false to let the key live forever.
  3. I renamed the instance variable
  4. The builder initialization is used to setup a stack of proxies (similar to rack). If you use an adapter without expiration support (e.g adapter :Memory), you have to add the Expires middleware (use :Expires). For Redis the native expiration is used. It is similar to what you did in your Memory backend (checking expiration in #get).

@minad
Copy link
Contributor Author

minad commented Jan 21, 2013

Ah just an idea: If you want to switch to Moneta but want to keep your interface you could create a module with (#get, #set and #flush) and include it into Moneta::Defaults. But I would deprecate the methods then.

@minad
Copy link
Contributor Author

minad commented Jan 21, 2013

And another idea: You could also make your store system Moneta compatible and only provide Memory. Then you could just drop in Moneta backends and remove Redis, Mongo etc. This would be how I would do it 😁

@ujifgc
Copy link
Member

ujifgc commented Jan 22, 2013

Since it's autoload everywhere, it won't impact memory footprint or load time. Tilt is thicker with its multiple template
inheritance.

Also we could store sessions with moneta.

@minad
Copy link
Contributor Author

minad commented Jan 22, 2013

@ujifgc : For sessions moneta includes Rack::Session::Moneta

@skade
Copy link
Contributor

skade commented Jan 22, 2013

@dariocravero From the perspective of Padrino, Moneta only provides another storage backend that needs to be configured. In a sense, it behaves like Sequel, AR and DataMapper in that respect. So, conceptually, I'll accept that.

Beyond @dariocravero's criticism, the pull request looks very clean and is up to our expectations.

When it comes to replacing the whole layer, thats a bit against the idea of Padrino: we provide the adapters and try not to force any further abstractions. If someone wants to plug a raw hash in there. We could endorse Moneta, though.

@minad
Copy link
Contributor Author

minad commented Jan 22, 2013

Ok. But I don't think you are completely right about "further abstractions". If you replace Padrino::Cache::Store it is just one abstraction replaced with another one.

The only difference that I see is that Moneta uses different constructors compared to your Padrino::Cache::Store. In your implementation it is really only an adapter, in Moneta it does a bit more. But I think it covers most use cases.

But if you want one could add constructors like Moneta.new(:Redis, :backend => Redis.new(...)) to Moneta to make it completly similar to your implementation.

@skade
Copy link
Contributor

skade commented Jan 22, 2013

I am aware that you switch out one adapter of similar levels for the other. The problem is that the current one is the Padrino adapter - we control the interface trough any number of versions, we can document it and we don't have to refer to the other projects docs.

This is already a big problem with Sinatra itself: most new users are actually not aware that all features (- some odd ones) work in Padrino and need to be looked up in the Sinatra docs.

So, if we have a component that interacts directly with the framework, like the caching layer, I prefer our framework to code against and not some third party, however well-behaving it might be. Getting bugs fixed at a third party is always a huge time cost compared to own, even more so for interface changes. Endorsing the project as a very good option is the way to go without introducing more hard dependencies.

@dariocravero
Copy link

I agree with @skade, Padrino should remain agnostic and Moneta a new cache store.

Thanks for the change @minad. Before merging it, perhaps we should add a note in the code or the docs telling users how the expiring mechanism works? This method doesn't really use expires_in -1 -even though it defaults to never expiring- and that may confuse some people. It's not a big deal anyway but a nice to have.

@minad
Copy link
Contributor Author

minad commented Jan 22, 2013

@skade, @dariocravero I know the problem with external dependencies well. And you are right about the communication effort etc. I only proposed it since it Cache::Store is just exactly the same thing like Moneta :)

@nesquena
Copy link
Member

Close this in favor of #1018 I think?

@nesquena nesquena closed this Jan 24, 2013
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.

None yet

5 participants