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

Cleanup work for Adapter#get_multi #206

Closed
jnunemaker opened this Issue Nov 25, 2016 · 7 comments

Comments

Projects
None yet
3 participants
@jnunemaker
Owner

jnunemaker commented Nov 25, 2016

get_multi was recently added, but only made efficient for the redis adapter. I need to finish making the other adapters do bulk loads for get multi instead of n+1's.

  • add get_multi to active record
  • add get_multi to dalli
  • add get_multi to mongo
  • add get_multi to sequel
  • return value for get_multi? currently array, should it be hash
  • ensure get_multi is doc'd (Adapters.md)
  • ensure preload and preload_all are doc's (Optimizations.md)

@jnunemaker jnunemaker self-assigned this Nov 25, 2016

@gshutler

This comment has been minimized.

Contributor

gshutler commented Nov 25, 2016

get_multi returning a Hash is probably preferable. I know it would reduce the complexity in Memoizable for example, and if you only want the Array you can always call values on the Hash so I don't think there's any loss.

Let me know if I can help in any way.

@jnunemaker

This comment has been minimized.

Owner

jnunemaker commented Nov 29, 2016

@gshutler cool. Changed to hash. I think I have everything ready but the docs. If you want to take a crack at those, feel free (just let me know so we don't both work on them), but I'll try to get to them soon and push out a new release afterwards.

@jnunemaker

This comment has been minimized.

Owner

jnunemaker commented Nov 29, 2016

@gshutler it would be helpful if you wanted to double check some of my recent changes for get_multi in the various adapters for sanity, but don't feel like you have to. I should have done a PR...

@gshutler

This comment has been minimized.

Contributor

gshutler commented Nov 29, 2016

I had a look through them all (using fc6b58d...master) and didn't notice anything unexpected. All following a similar pattern to the Redis one which only changed slightly to return a Hash.

I'm not going to have time to look at docs for a few days so unlikely to get to it before you do.

@jnunemaker

This comment has been minimized.

Owner

jnunemaker commented Nov 29, 2016

@gshutler thanks! No problem about the docs. I'll try to get to it tonight or tomorrow night.

@mscoutermarsh

This comment has been minimized.

mscoutermarsh commented Dec 8, 2016

Hey @jnunemaker. Running this in production now.

image

Nice. THANK YOU for your work on this.

This is an endpoint doing 10-15 flag checks with postgres. Now uses preload with redis.

❤️

@jnunemaker

This comment has been minimized.

Owner

jnunemaker commented Dec 8, 2016

Finished up docs this morning quick. Going to do a release.

@mscoutermarsh awesome! Really appreciate you sharing that.

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