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

Preserve key order of #fetch_multi #34700

Merged
merged 1 commit into from Jan 7, 2019

Conversation

@gmcgibbon
Copy link
Member

@gmcgibbon gmcgibbon commented Dec 13, 2018

Summary

Fixes #34694.

Preserves order of keys passed to #fetch_multi.

If for some reason the current functionality is intended, I'll refactor this PR to document the behaviour.

jeremy
jeremy approved these changes Dec 13, 2018
Copy link
Member

@jeremy jeremy left a comment

Nice @gmcgibbon!

@@ -1,3 +1,7 @@
* Preserve key order of `ActiveSupport::CacheStore#fetch_multi`.
Copy link
Member

@jeremy jeremy Dec 13, 2018

Let's illuminate this a bit, e.g. fetch_multi(*names) returns its results in the same order as the *names requested, rather than returning cache hits followed by cache misses.

@gmcgibbon gmcgibbon force-pushed the fetch_multi_key_order branch from a8915d3 to 4597418 Dec 13, 2018

write_multi writes, options
end
names.map { |name| [name, reads.fetch(name) { writes[name] }] }.to_h
Copy link
Member

@kaspth kaspth Dec 18, 2018

Tried grokking this over multiple attempts, but I find the two maps quite confusing since we seem to do similar yet different processing.

What if we kept some of the old flow ala this?

instrument :read_multi, names, options do |payload|
  reads = read_multi_entries(names, options)
  payload[:hits] = reads.keys
  payload[:super_operation] = :fetch_multi

  writes  = {}
  ordered = names.each_with_object({}) do |name, hash|
    hash[name] = reads.fetch(name) { writes[name] = yield name }
  end

  write_multi writes, options
  ordered
end

(Probably ends up needing to be yield(name). Psst, also a nice case for index_with, but we can't use core extensions here.)

fetch_multi(*names) now returns its results in the same order
as the `*names` requested, rather than returning cache hits
followed by cache misses.
@gmcgibbon gmcgibbon force-pushed the fetch_multi_key_order branch from 4597418 to 94ecf95 Dec 27, 2018
@rafaelfranca rafaelfranca merged commit cd16413 into rails:master Jan 7, 2019
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants