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

Cleanup work for Adapter#get_multi #206

Closed
7 tasks done
jnunemaker opened this issue Nov 25, 2016 · 7 comments
Closed
7 tasks done

Cleanup work for Adapter#get_multi #206

jnunemaker opened this issue Nov 25, 2016 · 7 comments
Assignees

Comments

@jnunemaker
Copy link
Collaborator

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
Copy link
Contributor

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
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

@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
Copy link
Contributor

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
Copy link
Collaborator Author

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

@mscoutermarsh
Copy link

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
Copy link
Collaborator Author

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
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants