[Feature Request] initial value for ActiveSupport::Cache::MemCacheStore#increment and decrement

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

[Feature Request] initial value for ActiveSupport::Cache::MemCacheStore#increment and decrement

Aero Astro
Hi there.

I made a Pull Request to fix bug on ActiveSupport::Cache::MemCacheStore#increment
and decrement for initialization of not stored value.

https://github.com/rails/rails/pull/30716


Regarding initialization, the current document says as follows.

```
# Increment a cached value. This method uses the memcached incr atomic
# operator and can only be used on values written with the :raw option.
# Calling it on a value not stored with :raw will initialize that value
# to zero.
```

However, I would rather initialize the value with `amount` for increment and `- amount` for decrement
as if the not stored value is initialized to 0 prior to the specified operation.

```
# Before
@data.incr(normalize_key(name, options), amount, options[:expires_in], 0)
@data.decr(normalize_key(name, options), amount, options[:expires_in], 0)

# After
@data.incr(normalize_key(name, options), amount, options[:expires_in], amount)
@data.decr(normalize_key(name, options), amount, options[:expires_in], -amount)
```


The reason is that we want to implement appropriate lazy initialization.
For example, simple counter with lazy initialization can be written with instance variable as follows.

```
def increment(amount)
  @counter ||= 0
  @counter += amount
end
```

If we implement the above counter with current ActiveSupport::Cache::MemCacheStore, it can be written as follows.

```
def increment(amount)
  result = @cache.increment(amount)
  result != 0 ? result : @cache.increment(amount)
end
```

As you can see, the initialization to 0 inside @cache actually does not help us write lazy initialization.
We should still handle 0 instead of nil. What is worse, if we use increment and decrement concurrently
on the same key, there is no way to calculate the accurate count.

With the proposed feature request the above code can be simplified as follows.

```
def increment(amount)
  @cache.increment(amount)
end
```

Also, I have found the same specification on incr of Redis.
https://redis.io/commands/incr

If the idea is O.K., I will submit another PR or change the current PR to address the issue.
IMHO, as this initialization has been disabled for a long time, it is about time to improve the specification.

--
Best Regards,
Takumasa Ochi

--
You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
Visit this group at https://groups.google.com/group/rubyonrails-core.
For more options, visit https://groups.google.com/d/optout.
Reply | Threaded
Open this post in threaded view
|

Re: [Feature Request] initial value for ActiveSupport::Cache::MemCacheStore#increment and decrement

Jeremy Daer
+1 to matching Redis-style INCR. Returning `nil` is useful in some cases, like lazy-initializing a counter to a value that's expensive to calculate, but for typical use it's a strange hassle.

Note that Memcached doesn't support negative counters, so `@data.decr(normalize_key(name, options), amount, options[:expires_in], -amount)` may have unexpected results (setting to 0).


On Monday, September 25, 2017 at 11:40:13 PM UTC-7, Aero Astro wrote:
Hi there.

I made a Pull Request to fix bug on ActiveSupport::Cache::MemCacheStore#increment
and decrement for initialization of not stored value.

<a href="https://github.com/rails/rails/pull/30716" target="_blank" rel="nofollow" onmousedown="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fgithub.com%2Frails%2Frails%2Fpull%2F30716\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNHY7_4XtwILHVOR9ZFOjXB-EALQ_g&#39;;return true;" onclick="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fgithub.com%2Frails%2Frails%2Fpull%2F30716\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNHY7_4XtwILHVOR9ZFOjXB-EALQ_g&#39;;return true;">https://github.com/rails/rails/pull/30716


Regarding initialization, the current document says as follows.

```
# Increment a cached value. This method uses the memcached incr atomic
# operator and can only be used on values written with the :raw option.
# Calling it on a value not stored with :raw will initialize that value
# to zero.
```

However, I would rather initialize the value with `amount` for increment and `- amount` for decrement
as if the not stored value is initialized to 0 prior to the specified operation.

```
# Before
@data.incr(normalize_key(name, options), amount, options[:expires_in], 0)
@data.decr(normalize_key(name, options), amount, options[:expires_in], 0)

# After
@data.incr(normalize_key(name, options), amount, options[:expires_in], amount)
@data.decr(normalize_key(name, options), amount, options[:expires_in], -amount)
```


The reason is that we want to implement appropriate lazy initialization.
For example, simple counter with lazy initialization can be written with instance variable as follows.

```
def increment(amount)
  @counter ||= 0
  @counter += amount
end
```

If we implement the above counter with current ActiveSupport::Cache::MemCacheStore, it can be written as follows.

```
def increment(amount)
  result = @cache.increment(amount)
  result != 0 ? result : @cache.increment(amount)
end
```

As you can see, the initialization to 0 inside @cache actually does not help us write lazy initialization.
We should still handle 0 instead of nil. What is worse, if we use increment and decrement concurrently
on the same key, there is no way to calculate the accurate count.

With the proposed feature request the above code can be simplified as follows.

```
def increment(amount)
  @cache.increment(amount)
end
```

Also, I have found the same specification on incr of Redis.
<a href="https://redis.io/commands/incr" target="_blank" rel="nofollow" onmousedown="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fredis.io%2Fcommands%2Fincr\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNFmvP_5ZQ9BaHoeBYBMbGzaGrraIg&#39;;return true;" onclick="this.href=&#39;https://www.google.com/url?q\x3dhttps%3A%2F%2Fredis.io%2Fcommands%2Fincr\x26sa\x3dD\x26sntz\x3d1\x26usg\x3dAFQjCNFmvP_5ZQ9BaHoeBYBMbGzaGrraIg&#39;;return true;">https://redis.io/commands/incr

If the idea is O.K., I will submit another PR or change the current PR to address the issue.
IMHO, as this initialization has been disabled for a long time, it is about time to improve the specification.

--
Best Regards,
Takumasa Ochi

--
You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [hidden email].
To post to this group, send email to [hidden email].
Visit this group at https://groups.google.com/group/rubyonrails-core.
For more options, visit https://groups.google.com/d/optout.