Marshaling #10
Marshaling #10
Conversation
lib/atomic_cache/storage/dalli.rb
Outdated
| @@ -33,12 +33,14 @@ def add(key, new_value, ttl, user_options=nil) | |||
|
|
|||
| def read(key, user_options=nil) | |||
| user_options ||= {} | |||
| @dalli_client.read(key, user_options) | |||
| raw = @dalli_client.read(key, user_options) | |||
There was a problem hiding this comment.
What do you think about naming this variable marshaled ? It might be more clear.
On the other hand, it looks like the marshal method might not marshal the objects depending on the options. I wonder if you might change the structure to only call marshal if it should be done. That could make the code slightly more understandable.
lib/atomic_cache/storage/dalli.rb
Outdated
| @@ -33,12 +33,14 @@ def add(key, new_value, ttl, user_options=nil) | |||
|
|
|||
| def read(key, user_options=nil) | |||
There was a problem hiding this comment.
should user_options default to {} instead of nil?
Alternatively, do keyword args make more sense?
There was a problem hiding this comment.
keyword args would add a level of explicitness, which I think would be a good improvement
There was a problem hiding this comment.
Ah, actually, I remember now why I didn't do that originally. It's because these options end up getting passed into the dalli client. If there was a way to say "I expect these options, but also take a hash of unknown options that I'll just pass on" that'd be cool, but I wasn't aware of such a language mechanic.
There was a problem hiding this comment.
Just to note, use of kwargs would drop support for Ruby < 2.0. But 1.9.3 has been EOL'ed so I don't think we will need to worry about it.
There was a problem hiding this comment.
Would raw impact any of the downstream clients? I wonder if we want to explicitly separate those like cache_client_options and then raw kwarg for things we understand?
There was a problem hiding this comment.
Regarding =nil vs ={}, the one challenge I see to doing ={} is if nil gets passed in, you don't automatically get a {} out of it:
irb(main):001:0> def foo(arg={})
irb(main):002:1> puts arg
irb(main):003:1> end
=> :foo
irb(main):004:0> foo(nil)
=> nil
vs.
irb(main):005:0> def foo2(arg=nil)
irb(main):006:1> arg ||= {}
irb(main):007:1> puts arg
irb(main):008:1> end
=> :foo2
irb(main):009:0> foo2(nil)
{}
I think one argument in favor of ={} is that if the user is explicitly passing in nil, then maybe an exception is the right response there, as otherwise the latter form may be hiding a bug. Thoughts?
There was a problem hiding this comment.
I definitely see your point that this requires an additional guard clause to prevent errors from bubbling up. However, I tend to agree that if the documentation says you should pass something Hash-y and you pass nil, you're in violation of the API.
I don't feel strongly on the nil || {} issue, so feel free to go with what feels best to you :-)
…tions will now cause an exception.
| @@ -37,8 +37,7 @@ def initialize(storage: nil, timestamp_manager: nil, default_options: {}, logger | |||
| # @option options [Numeric] :max_retries (5) Max times to rety in waiting case | |||
| # @option options [Numeric] :backoff_duration_ms (50) Duration in ms to wait between retries | |||
| # @yield Generates a new value when cache is expired | |||
| def fetch(keyspace, options=nil) | |||
| options ||= {} | |||
| def fetch(keyspace, options={}) | |||
There was a problem hiding this comment.
All options input from the user was changed from options=nil to options={}. In the case where the user passes in a nil, that's an error case and an exception will be encountered. As it was, by doing =nil then ||= {} it was silently hiding error cases. If something is passed in as options, it needs to be hash-like.
| @@ -26,6 +26,18 @@ def set(key, new_value, user_options); raise NotImplementedError end | |||
| # returns true if it succeeds; false otherwise | |||
| def delete(key, user_options); raise NotImplementedError end | |||
|
|
|||
| protected | |||
|
|
|||
| def marshal(value, user_options={}) | |||
There was a problem hiding this comment.
curious, but would we not assume that the underlying cache impl is marshaling for us?
https://github.com/petergoldstein/dalli/blob/master/lib/dalli/client.rb#L29
https://github.com/petergoldstein/dalli/blob/master/lib/dalli/server.rb#L408
There was a problem hiding this comment.
That is exactly what I assumed when I first wrote this, which is why it lacked marshaling the first time around. When Lucas and I started on the first integration, it was clear that the previous Rails.cache.fetch was doing serialization, and AtomicCache was not. Granted, we were utilizing the in-memory storage adapter, so at a minimum that likely needs to support it.
Since I knew it needed to be added for in-memory, I poked around the Rails source to see if they were handling it for their version of the memcache client, which is when I turned up this: https://github.com/rails/rails/blob/master/activesupport/lib/active_support/cache/mem_cache_store.rb#L194
I had implemented marshaling for both the in-memory and the dalli storage adapter, primarily because I was inferring it was needed based on the Rails source.
So, "curious" indeed. Based on the Dalli source you've linked to, I'd be fine pulling out marshaling for the Dalli storage adapter, but leaving it in for the in-memory adapter.
Adds support for marshaling/unmarshaling complex objects into and out of cache. This elevates behavior to be in parity with
Rails.cache.fetchwhich is automatically handling this.As far as I can tell, marshaling/unmarshaling seems to be implemented per storage adapter in ActiveSupport (example). Ruby's
Marshalseems to be the canonical answer.Along similar lines, I noticed that compression is being implemented in the same fashion, so it may be necessary to provide compression support manually as well. There doesn't seem to be a common concern among the ActiveSupport cache implementations that just provides compression and marshaling across the board.