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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use $MEMCACHE_SERVERS
as MemCacheStore
fallback
#40420
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,7 +45,6 @@ def setup | |
@namespace = "test-#{SecureRandom.hex}" | ||
@cache = lookup_store(expires_in: 60) | ||
@peek = lookup_store | ||
@data = @cache.instance_variable_get(:@data) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I couldn't find anywhere this was used in this file or any of the included modules, and this file's tests continued to pass for me locally. 馃し |
||
@cache.silence! | ||
@cache.logger = ActiveSupport::Logger.new(File::NULL) | ||
end | ||
|
@@ -64,7 +63,6 @@ def setup | |
# Overrides test from LocalCacheBehavior in order to stub out the cache clear | ||
# and replace it with a delete. | ||
def test_clear_also_clears_local_cache | ||
client = @cache.instance_variable_get(:@data) | ||
key = "#{@namespace}:foo" | ||
client.stub(:flush_all, -> { client.delete(key) }) do | ||
super | ||
|
@@ -102,14 +100,14 @@ def test_local_cache_raw_values | |
|
||
def test_increment_expires_in | ||
cache = lookup_store(raw: true, namespace: nil) | ||
assert_called_with cache.instance_variable_get(:@data), :incr, [ "foo", 1, 60 ] do | ||
assert_called_with client(cache), :incr, [ "foo", 1, 60 ] do | ||
cache.increment("foo", 1, expires_in: 60) | ||
end | ||
end | ||
|
||
def test_decrement_expires_in | ||
cache = lookup_store(raw: true, namespace: nil) | ||
assert_called_with cache.instance_variable_get(:@data), :decr, [ "foo", 1, 60 ] do | ||
assert_called_with client(cache), :decr, [ "foo", 1, 60 ] do | ||
cache.decrement("foo", 1, expires_in: 60) | ||
end | ||
end | ||
|
@@ -164,18 +162,47 @@ def test_no_multiple_compress | |
|
||
def test_unless_exist_expires_when_configured | ||
cache = ActiveSupport::Cache.lookup_store(:mem_cache_store) | ||
assert_called_with cache.instance_variable_get(:@data), :add, [ "foo", ActiveSupport::Cache::Entry, 1, Hash ] do | ||
assert_called_with client(cache), :add, [ "foo", ActiveSupport::Cache::Entry, 1, Hash ] do | ||
cache.write("foo", "bar", expires_in: 1, unless_exist: true) | ||
end | ||
end | ||
|
||
def test_uses_provided_dalli_client_if_present | ||
cache = ActiveSupport::Cache.lookup_store(:mem_cache_store, Dalli::Client.new("custom_host")) | ||
|
||
assert_equal ["custom_host"], servers(cache) | ||
end | ||
|
||
def test_forwards_string_addresses_if_present | ||
expected_addresses = ["first", "second"] | ||
cache = ActiveSupport::Cache.lookup_store(:mem_cache_store, expected_addresses) | ||
|
||
assert_equal expected_addresses, servers(cache) | ||
end | ||
|
||
def test_falls_back_to_localhost_if_no_address_provided_and_memcache_servers_undefined | ||
with_memcache_servers_environment_variable(nil) do | ||
cache = ActiveSupport::Cache.lookup_store(:mem_cache_store) | ||
|
||
assert_equal ["127.0.0.1:11211"], servers(cache) | ||
end | ||
end | ||
|
||
def test_falls_back_to_localhost_if_no_address_provided_and_memcache_servers_defined | ||
with_memcache_servers_environment_variable("custom_host") do | ||
cache = ActiveSupport::Cache.lookup_store(:mem_cache_store) | ||
|
||
assert_equal ["custom_host"], servers(cache) | ||
end | ||
end | ||
|
||
private | ||
def random_string(length) | ||
(0...length).map { (65 + rand(26)).chr }.join | ||
end | ||
|
||
def store | ||
[:mem_cache_store, ENV["MEMCACHE_SERVERS"] || "localhost:11211"] | ||
[:mem_cache_store] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seemed redundant not to leverage the code I just added 馃槄 |
||
end | ||
|
||
def emulating_latency | ||
|
@@ -197,4 +224,24 @@ def emulating_unavailability | |
Dalli.send(:remove_const, :Server) | ||
Dalli.const_set(:Server, old_server) | ||
end | ||
|
||
def servers(cache = @cache) | ||
client(cache).instance_variable_get(:@servers) | ||
end | ||
|
||
def client(cache = @cache) | ||
cache.instance_variable_get(:@data) | ||
end | ||
Comment on lines
+228
to
+234
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't feel great about doing this, but neither of these are publicly accessible, and we were already using |
||
|
||
def with_memcache_servers_environment_variable(value) | ||
original_value = ENV["MEMCACHE_SERVERS"] | ||
ENV["MEMCACHE_SERVERS"] = value | ||
yield | ||
ensure | ||
if original_value.nil? | ||
ENV.delete("MEMCACHE_SERVERS") | ||
Comment on lines
+241
to
+242
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might not be needed, but I figured it would make sense to remove it as a key entirely if it wasn't there originally. |
||
else | ||
ENV["MEMCACHE_SERVERS"] = original_value | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,7 @@ | ||
* Updated `ActiveSupport::Cache::MemCacheStore` docs to reflect support for `$MEMCACHE_SERVERS`. | ||
|
||
*Sam Bostock* | ||
|
||
Comment on lines
+1
to
+4
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if this is required. I couldn't find anything describing what changes to the Guides require CHANGELOG entries. |
||
* Use Bookstore as a unified use-case for all examples in Active Record Query Interface Guide. | ||
|
||
*Ashley Engelund*, *Vipul A M* | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Users of
:dalli_store
may have been passing an explicitnil
parameter for the servers:If they simply changed
:dalli_cache
and:mem_cache_store
, this code passesaddresses = [nil]
to Dalli (instead ofnil
, which cause exceptions when people try to access the cache.I've seen at least 2 cases within repos where this was the case, which confused hapless developers who were just trying to migrate in order to clear the deprecation warning (causing them to revert instead of debugging the issue).
Perhaps we could do something like add a
.compact
:This would unify the interface between the two stores.
Or perhaps raise an exception that explained what they were doing wrong / the difference in interface between
:dalli_store
and:mem_cache_store
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to explain, but I think what needs to explain how the migration should be done is the dalli_store project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of some relevance, I opened petergoldstein/dalli#764 a while back to document the change in implicit ENV variable handling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think the
compact
version is fine. @movermeyer want to open a PR?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rafaelfranca It would be my pleasure!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#41381