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

replace Padrino::Cache::Store with Moneta #1018

Merged
merged 1 commit into from Oct 15, 2013

Conversation

Projects
None yet
7 participants
@minad
Contributor

minad commented Jan 22, 2013

As an alternative to #1017 I replaced Padrino::Cache::Store completely with Moneta.

@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Jan 22, 2013

Member

See #1017 (comment) in #1017 for my opinion on this. I see more hard third-party dependencies as problematic. Vendoring might be another way to go.

Still, I don't want to block this change - Moneta is a solution fitting the problem and allows us to deprecate some of our stores in exchange for depending on another third-party. Given that we are the "rack lovers" framework, its integration into Rack is very nice.

What I do want to block: the change is needlessly backwards-incompatible (fine for a discussion). If accepted, an API change with deprecation notice is necessary. But this is not alot of work.

@padrino/ecore-members , especially those who wrote Cache and use it alot: is the gained flexibily and ease of development worth throwing out Cache completely? And how do we allow users to plugin in something different?

Member

skade commented Jan 22, 2013

See #1017 (comment) in #1017 for my opinion on this. I see more hard third-party dependencies as problematic. Vendoring might be another way to go.

Still, I don't want to block this change - Moneta is a solution fitting the problem and allows us to deprecate some of our stores in exchange for depending on another third-party. Given that we are the "rack lovers" framework, its integration into Rack is very nice.

What I do want to block: the change is needlessly backwards-incompatible (fine for a discussion). If accepted, an API change with deprecation notice is necessary. But this is not alot of work.

@padrino/ecore-members , especially those who wrote Cache and use it alot: is the gained flexibily and ease of development worth throwing out Cache completely? And how do we allow users to plugin in something different?

@minad

This comment has been minimized.

Show comment
Hide comment
@minad

minad Jan 22, 2013

Contributor

@skade: I agree, this fits well.

Backward incompatibility: What is missing to make it compatible? As I see it the only thing missing is the constructors Padrino::Cache::Store::Memory.new etc. The #get, #set and #flush methods are provided by the LegacyStore mixin.

Plugging in something different: This is really easy. You only have to write something with a Moneta compatible interface. I also try to keep Moneta very up-to-date, this means that it should support virtually all available key/value stores. So I think the case that someone needs something else would seldomly come up, but then I would be happy to merge the adapter into Moneta.

Contributor

minad commented Jan 22, 2013

@skade: I agree, this fits well.

Backward incompatibility: What is missing to make it compatible? As I see it the only thing missing is the constructors Padrino::Cache::Store::Memory.new etc. The #get, #set and #flush methods are provided by the LegacyStore mixin.

Plugging in something different: This is really easy. You only have to write something with a Moneta compatible interface. I also try to keep Moneta very up-to-date, this means that it should support virtually all available key/value stores. So I think the case that someone needs something else would seldomly come up, but then I would be happy to merge the adapter into Moneta.

@minad

This comment has been minimized.

Show comment
Hide comment
@minad

minad Jan 22, 2013

Contributor

And towards your argument against third-party-code - If someone of you @padrino/core-members wants to contribute to Moneta, I will happily provide commit access. The only problem remaining is that the releases are not synchronized.

Contributor

minad commented Jan 22, 2013

And towards your argument against third-party-code - If someone of you @padrino/core-members wants to contribute to Moneta, I will happily provide commit access. The only problem remaining is that the releases are not synchronized.

@dariocravero

This comment has been minimized.

Show comment
Hide comment
@dariocravero

dariocravero Jan 23, 2013

Contributor

I use cache quite a lot and haven't found any big issues with the way Padrino is managing that so far. Having say that, I reckon that using a gem like Moneta, which is highly specialised on the topic perhaps makes more sense. At the end of the day it will allow us to grow our cache-stores offer without the overhead of having to implement them. That's a big plus to me.

To sum up, I'm fine with this implementation which still keeps the helpers but replaces the underlaying caching implementation.

@DAddYE @nesquena?

Contributor

dariocravero commented Jan 23, 2013

I use cache quite a lot and haven't found any big issues with the way Padrino is managing that so far. Having say that, I reckon that using a gem like Moneta, which is highly specialised on the topic perhaps makes more sense. At the end of the day it will allow us to grow our cache-stores offer without the overhead of having to implement them. That's a big plus to me.

To sum up, I'm fine with this implementation which still keeps the helpers but replaces the underlaying caching implementation.

@DAddYE @nesquena?

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Jan 23, 2013

Member

On the one hand, Moneta does remind me of rack and tilt and in cases of clean, well-scoped standard libraries there can be a big advantage to using those or wrapping them instead of reinventing. On the other, as @skade mentioned it does introduce a hard dependency and another place users have to go to read documentation (altho we can mitigate some of that).

Looking at the code, I also would want to make sure we don't make things needlessly backwards incompatible. From the commit it looks like all the helpers and cache interface would stay the same (with some deprecation notices) and the only serious backwards incompatible change is when you set the cache adapter (Padrino::Cache::Store::Memcache.new becomes Moneta.new)?

What happens when someone upgrades their application and they haven't changed the caching adapter? I guess they would be greeted with the error that the class doesn't exist. Do we think this is fine for a major (0.11.0) release?

I am often glad to be able to remove code to maintain as part of Padrino especially because when moneta itself has no additional hard dependencies. It does seem to cleanup a lot of code that we no longer have to maintain. I think it's a positive change. Thoughts? /cc @achiu @DAddYE

Member

nesquena commented Jan 23, 2013

On the one hand, Moneta does remind me of rack and tilt and in cases of clean, well-scoped standard libraries there can be a big advantage to using those or wrapping them instead of reinventing. On the other, as @skade mentioned it does introduce a hard dependency and another place users have to go to read documentation (altho we can mitigate some of that).

Looking at the code, I also would want to make sure we don't make things needlessly backwards incompatible. From the commit it looks like all the helpers and cache interface would stay the same (with some deprecation notices) and the only serious backwards incompatible change is when you set the cache adapter (Padrino::Cache::Store::Memcache.new becomes Moneta.new)?

What happens when someone upgrades their application and they haven't changed the caching adapter? I guess they would be greeted with the error that the class doesn't exist. Do we think this is fine for a major (0.11.0) release?

I am often glad to be able to remove code to maintain as part of Padrino especially because when moneta itself has no additional hard dependencies. It does seem to cleanup a lot of code that we no longer have to maintain. I think it's a positive change. Thoughts? /cc @achiu @DAddYE

@minad

This comment has been minimized.

Show comment
Hide comment
@minad

minad Jan 24, 2013

Contributor

Are you already working on 1.0 or do you want to keep this open until then?

If you want I could also make it completly backwards compatible with some minor effort (adding the missing classes/fake constructors).

Contributor

minad commented Jan 24, 2013

Are you already working on 1.0 or do you want to keep this open until then?

If you want I could also make it completly backwards compatible with some minor effort (adding the missing classes/fake constructors).

@nesquena nesquena referenced this pull request Jan 24, 2013

Closed

add moneta cache store #1017

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Jan 24, 2013

Member

We actually want to put this in possibly pre-1.0 but after the very next release which we want to cut soon. Leave this open for now, and we will soon merge in post 0.11.0 (0.12.0? @DAddYE). Thanks

Member

nesquena commented Jan 24, 2013

We actually want to put this in possibly pre-1.0 but after the very next release which we want to cut soon. Leave this open for now, and we will soon merge in post 0.11.0 (0.12.0? @DAddYE). Thanks

@minad

This comment has been minimized.

Show comment
Hide comment
@minad

minad Jan 24, 2013

Contributor

Ok, cool 👍

Contributor

minad commented Jan 24, 2013

Ok, cool 👍

@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Jan 25, 2013

Member

This sounds like a good direction.

Member

skade commented Jan 25, 2013

This sounds like a good direction.

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE Jan 26, 2013

Member

We should discuss a bit more about that, @ujifgc and @skade can we check how much ram we need for that? Performances?

Our goal for next version is keep things simple, clean and concise.

Using an external gem our code definitely will look clear but people to understand what we are doing should browse moneta sources which actually is great but are a huge amount of files ... 😸

So instead, I propose:

  • Keep ours padrino-cache
  • Simplify wherever we can it
  • Add an api (@minad can do that) for a super simple interface to moneta.

Make sense? Thoughts ?

Member

DAddYE commented Jan 26, 2013

We should discuss a bit more about that, @ujifgc and @skade can we check how much ram we need for that? Performances?

Our goal for next version is keep things simple, clean and concise.

Using an external gem our code definitely will look clear but people to understand what we are doing should browse moneta sources which actually is great but are a huge amount of files ... 😸

So instead, I propose:

  • Keep ours padrino-cache
  • Simplify wherever we can it
  • Add an api (@minad can do that) for a super simple interface to moneta.

Make sense? Thoughts ?

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Jan 26, 2013

Member

moneta uses ruby autoload to require it's backends, so, it's very tidy and uses no excess memory over padrino-cache.

Member

ujifgc commented Jan 26, 2013

moneta uses ruby autoload to require it's backends, so, it's very tidy and uses no excess memory over padrino-cache.

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE Jan 26, 2013

Member

yep but autoload is not threadsafe and dead in ruby 2.0 :(

Member

DAddYE commented Jan 26, 2013

yep but autoload is not threadsafe and dead in ruby 2.0 :(

@minad

This comment has been minimized.

Show comment
Hide comment
@minad

minad Jan 27, 2013

Contributor

@DAddYE

Your proposol: Well, then you could just take the other pull request.

About your criticism of Moneta:

  • Moneta uses autoload yes, but this can be changed as soon as ruby 3.0 is there. Ruby 2.0 will still support it. And when it is really removed I will just implement autoloading manually or use a pattern that other libraries use in the same case.
  • Performance/Memory: Well I cannot say much about that, but if you only load moneta only a few constants will be created. The performance of the key/value stores is ensured by continous benchmarking on travis-ci. The moneta adapters itself are very thin and have an implementation similar to yours.
  • Moneta has many classes. This is true, but most of the classes are just adapters for the different backends. Moneta is also very feature rich, supports configurable value compression and serialization. This is all hidden behind the simple Moneta.new interface, but a power user could access all these niceties. I would also say that the design is quite clean since a lot of things are handled in different layers, similar to rack middlewares.

Moneta does also have a very extensive test and benchmarking suite. So this pull request is not only about reducing code, but also about ensuring code quality, adding many backends and other powerful features, ...

Maybe some words about my motiviation for this request: I would really like projects to profit from each other. I think this could be the case here. And as I said I am open to any suggestions and you can also contribute at the Moneta project (commit rights etc) if you want to.

So think about it :)

Contributor

minad commented Jan 27, 2013

@DAddYE

Your proposol: Well, then you could just take the other pull request.

About your criticism of Moneta:

  • Moneta uses autoload yes, but this can be changed as soon as ruby 3.0 is there. Ruby 2.0 will still support it. And when it is really removed I will just implement autoloading manually or use a pattern that other libraries use in the same case.
  • Performance/Memory: Well I cannot say much about that, but if you only load moneta only a few constants will be created. The performance of the key/value stores is ensured by continous benchmarking on travis-ci. The moneta adapters itself are very thin and have an implementation similar to yours.
  • Moneta has many classes. This is true, but most of the classes are just adapters for the different backends. Moneta is also very feature rich, supports configurable value compression and serialization. This is all hidden behind the simple Moneta.new interface, but a power user could access all these niceties. I would also say that the design is quite clean since a lot of things are handled in different layers, similar to rack middlewares.

Moneta does also have a very extensive test and benchmarking suite. So this pull request is not only about reducing code, but also about ensuring code quality, adding many backends and other powerful features, ...

Maybe some words about my motiviation for this request: I would really like projects to profit from each other. I think this could be the case here. And as I said I am open to any suggestions and you can also contribute at the Moneta project (commit rights etc) if you want to.

So think about it :)

@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Jan 27, 2013

Member

I think the big advantage of using moneta is one of seperation of concerns. autoload is problematic (and for that reason, Rails just removed it, as it has bitten them), but when we adopt moneta, thats monetas problem :). Then again, my main complaint here is overreliance on constant names:

A fix for this problem would be not to rely on the constants directly on the Padrino side and let moneta handle that. E.g.:

set :cache, :adapter => :redis, :db => 1 #....
set :cache_instance, Proc.new { @instance ||= Moneta.new(cache.delete(:adapter), cache) } #maybe simplify that?

And a similar interface on the Moneta side. This allows Moneta to load the constant on first access without having to rely on magically appearing constants. Such interfaces are generally preferable, as they allow lazy loading in a much more decoupled manner.

About Memory footprint: we should bench that. I think the moneta project is also interested in getting better in that regard if possible.

Member

skade commented Jan 27, 2013

I think the big advantage of using moneta is one of seperation of concerns. autoload is problematic (and for that reason, Rails just removed it, as it has bitten them), but when we adopt moneta, thats monetas problem :). Then again, my main complaint here is overreliance on constant names:

A fix for this problem would be not to rely on the constants directly on the Padrino side and let moneta handle that. E.g.:

set :cache, :adapter => :redis, :db => 1 #....
set :cache_instance, Proc.new { @instance ||= Moneta.new(cache.delete(:adapter), cache) } #maybe simplify that?

And a similar interface on the Moneta side. This allows Moneta to load the constant on first access without having to rely on magically appearing constants. Such interfaces are generally preferable, as they allow lazy loading in a much more decoupled manner.

About Memory footprint: we should bench that. I think the moneta project is also interested in getting better in that regard if possible.

@minad

This comment has been minimized.

Show comment
Hide comment
@minad

minad Jan 27, 2013

Contributor

I don't understand what you mean my overreliance on constant names (:Redis vs :redis?). If you implement autoloading manually (not relying on"magically appearing constants", you have to deal with the same thread-safety problems. If you have suggestion on how to improve the moneta api, don't hesitate to create an issue.

But autoloading or not is really Monetas problem and your Store uses autoload too. Therefore this whole autoload discussion here doesn't make much sense, but the concern is noted.

Thread safety for autoloading the cache stores is not a big problem since it is done at startup (similar to new rails eager autoloading)

Contributor

minad commented Jan 27, 2013

I don't understand what you mean my overreliance on constant names (:Redis vs :redis?). If you implement autoloading manually (not relying on"magically appearing constants", you have to deal with the same thread-safety problems. If you have suggestion on how to improve the moneta api, don't hesitate to create an issue.

But autoloading or not is really Monetas problem and your Store uses autoload too. Therefore this whole autoload discussion here doesn't make much sense, but the concern is noted.

Thread safety for autoloading the cache stores is not a big problem since it is done at startup (similar to new rails eager autoloading)

@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Jan 27, 2013

Member

@minad I think there has been a misunderstanding: I meant the current API, which requires users to directly access constants, which doesn't allow for changing the loading in the back. Thats why I'd be wary with using Moneta.new directly, as it means that you cannot break API there.

About autoload in Moneta itself: I really don't care what color the bikeshed is, as long as its not mine and it harbors bikes :).

Concerning loading at bootstrap time: yes, thats precisely what I prefer and how its implemented is at the discretion of the library in use.

Member

skade commented Jan 27, 2013

@minad I think there has been a misunderstanding: I meant the current API, which requires users to directly access constants, which doesn't allow for changing the loading in the back. Thats why I'd be wary with using Moneta.new directly, as it means that you cannot break API there.

About autoload in Moneta itself: I really don't care what color the bikeshed is, as long as its not mine and it harbors bikes :).

Concerning loading at bootstrap time: yes, thats precisely what I prefer and how its implemented is at the discretion of the library in use.

@minad

This comment has been minimized.

Show comment
Hide comment
@minad

minad Jan 28, 2013

Contributor

@skade There are two api entry points to create moneta stores, but one could overwrite the proc :cache_instance in your suggestion.

Moneta.new(:Memory, :expires => true)

Moneta.build do
  use :Expires
  adapter :Memory
end

the second one is to add special middlewares.

Contributor

minad commented Jan 28, 2013

@skade There are two api entry points to create moneta stores, but one could overwrite the proc :cache_instance in your suggestion.

Moneta.new(:Memory, :expires => true)

Moneta.build do
  use :Expires
  adapter :Memory
end

the second one is to add special middlewares.

@minad

This comment has been minimized.

Show comment
Hide comment
@minad

minad Jan 30, 2013

Contributor

I added a memusage script to Moneta: https://github.com/minad/moneta/blob/master/script/memusage

Example output on 1.9.3:

# 29772K +29772K
require 'moneta'
# 29772K +0K
Moneta.new(:Memory)
# 30828K +1056K
Moneta.new(:File, :dir => 'filestore')
# 34940K +4112K
Moneta.new(:MemcachedNative)
# 68616K +33676K
Moneta.new(:MemcachedDalli)
# 84580K +15964K
Contributor

minad commented Jan 30, 2013

I added a memusage script to Moneta: https://github.com/minad/moneta/blob/master/script/memusage

Example output on 1.9.3:

# 29772K +29772K
require 'moneta'
# 29772K +0K
Moneta.new(:Memory)
# 30828K +1056K
Moneta.new(:File, :dir => 'filestore')
# 34940K +4112K
Moneta.new(:MemcachedNative)
# 68616K +33676K
Moneta.new(:MemcachedDalli)
# 84580K +15964K
@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Jan 30, 2013

Member

@minad what platform is it? 32 or 64 bit?

Member

ujifgc commented Jan 30, 2013

@minad what platform is it? 32 or 64 bit?

@minad

This comment has been minimized.

Show comment
Hide comment
@minad

minad Jan 30, 2013

Contributor

linux 64 bit

Contributor

minad commented Jan 30, 2013

linux 64 bit

@minad

This comment has been minimized.

Show comment
Hide comment
@minad

minad Jan 30, 2013

Contributor

@ujifgc You might want to test it with padrino loaded and see if it makes a difference.

Contributor

minad commented Jan 30, 2013

@ujifgc You might want to test it with padrino loaded and see if it makes a difference.

@minad

This comment has been minimized.

Show comment
Hide comment
@minad

minad Apr 9, 2013

Contributor

Will this go in at some point?

Contributor

minad commented Apr 9, 2013

Will this go in at some point?

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Apr 9, 2013

Member

I would definitely like it to, I think it will make the codebase for cache much leaner and easier to maintain by relying on moneta and not reinventing the wheel. The only question is of when in my mind. It is going to be a fairly breaking change, so I would say at this point on the next major point release i.e 0.12.0. Thoughts @padrino/core-members ?

Member

nesquena commented Apr 9, 2013

I would definitely like it to, I think it will make the codebase for cache much leaner and easier to maintain by relying on moneta and not reinventing the wheel. The only question is of when in my mind. It is going to be a fairly breaking change, so I would say at this point on the next major point release i.e 0.12.0. Thoughts @padrino/core-members ?

@dariocravero

This comment has been minimized.

Show comment
Hide comment
@dariocravero

dariocravero Apr 9, 2013

Contributor

👍!

Contributor

dariocravero commented Apr 9, 2013

👍!

@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Apr 10, 2013

Member

Next Major was also what I thought of.

Am 09.04.2013 um 23:52 schrieb Nathan Esquenazi notifications@github.com:

I would definitely like it to, I think it will make the codebase for cache much leaner and easier to maintain by relying on moneta and not reinventing the wheel. The only question is of when in my mind. It is going to be a fairly breaking change, so I would say at this point on the next major point release i.e 0.12.0. Thoughts @padrino/core-members ?


Reply to this email directly or view it on GitHub.

Member

skade commented Apr 10, 2013

Next Major was also what I thought of.

Am 09.04.2013 um 23:52 schrieb Nathan Esquenazi notifications@github.com:

I would definitely like it to, I think it will make the codebase for cache much leaner and easier to maintain by relying on moneta and not reinventing the wheel. The only question is of when in my mind. It is going to be a fairly breaking change, so I would say at this point on the next major point release i.e 0.12.0. Thoughts @padrino/core-members ?


Reply to this email directly or view it on GitHub.

@minad

This comment has been minimized.

Show comment
Hide comment
@minad

minad Sep 28, 2013

Contributor

I rebased the pull request on your master and added a few improvements. As discussed before the only difference to your implementation is the creation of the stores. Padrino::Cache::Store::Memory.new(50) is now Padrino::Cache.new(:Memory). You can also use Moneta.new(:Memory) directly but then you don't get the legacy interface.

I have seen that you had problems with encoding and marshalling (#821, #1128). The key/value transformation feature is also provided by Moneta directly.

Contributor

minad commented Sep 28, 2013

I rebased the pull request on your master and added a few improvements. As discussed before the only difference to your implementation is the creation of the stores. Padrino::Cache::Store::Memory.new(50) is now Padrino::Cache.new(:Memory). You can also use Moneta.new(:Memory) directly but then you don't get the legacy interface.

I have seen that you had problems with encoding and marshalling (#821, #1128). The key/value transformation feature is also provided by Moneta directly.

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Sep 28, 2013

Member

This is great, thanks for rebasing. Will be a part of 0.12

Member

nesquena commented Sep 28, 2013

This is great, thanks for rebasing. Will be a part of 0.12

@dariocravero

This comment has been minimized.

Show comment
Hide comment
@dariocravero

dariocravero Sep 30, 2013

Contributor

👍

Contributor

dariocravero commented Sep 30, 2013

👍

@minad

This comment has been minimized.

Show comment
Hide comment
Contributor

minad commented Oct 13, 2013

ujifgc added a commit that referenced this pull request Oct 15, 2013

Merge pull request #1018 from minad/moneta-replace
replace Padrino::Cache::Store with Moneta

@ujifgc ujifgc merged commit 3e551c9 into padrino:master Oct 15, 2013

1 check passed

default The Travis CI build passed
Details
@Litterfeldt

This comment has been minimized.

Show comment
Hide comment
@Litterfeldt

Litterfeldt Jun 17, 2014

Please update the documentation when doing changes like this!
http://www.padrinorb.com/guides/padrino-cache

Litterfeldt commented on 7609271 Jun 17, 2014

Please update the documentation when doing changes like this!
http://www.padrinorb.com/guides/padrino-cache

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Jun 21, 2014

Member

@Litterfeldt Yeah that was our mistake. I think we've updated it correctly now.

Member

nesquena replied Jun 21, 2014

@Litterfeldt Yeah that was our mistake. I think we've updated it correctly now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment