search

ShelbyTV / shelby_gt

Rails API services for Shelby GT
2 stars 0 forks source link

Framer improvements #207

Closed iceberg901 closed 10 years ago

iceberg901 commented 10 years ago

@spinosa @hsztul With any significant Framer change I like to get a code review on her before merging.

We figured out that n calls to save n new dashboard entries each try to check out their own socket, resulting in the use of n sockets.

As such, when the framer knows it will be creating and persisting multiple dashboard entries, it should persist them all at once using a call to the collection's insert method, which will do the entire operation over a single socket. I've implemented this here but because of the way we were previously doing things it was a little bit bigger than a tiny change.

Let me know if you see anything amiss.

I also moved the persist option into the options hash instead of having it as its own separate argument.

hsztul commented 10 years ago

Looks like you cleaned up things nicely @iceberg901

Henry

Entertainment Scientist http://tinyletter.com/sztul Shelby.tv @sztul http://twitter.com/sztul

On Tue, Nov 19, 2013 at 5:24 PM, iceberg901 notifications@github.comwrote:

@spinosa https://github.com/spinosa @hsztul https://github.com/hsztulWith any significant Framer change I like to get a code review on her before merging.

We figured out that n calls to save n new dashboard entries each try to check out their own socket, resulting in the use of n sockets.

As such, when the framer knows it will be creating and persisting multiple dashboard entries, it should persist them all at once using a call to the collection's insert method, which will do the entire operation over a single socket. I've implemented this here but because of the way we were previously doing things it was a little bit bigger than a tiny change.

Let me know if you see anything amiss.

I also moved the persist option into the options hash instead of having it

as its own separate argument.

You can merge this Pull Request by running

git pull https://github.com/ShelbyTV/shelby_gt framer-improvements

Or view, comment on, or merge it at:

https://github.com/ShelbyTV/shelby_gt/pull/207 Commit Summary

  • Make the persist option a member of the options hash instead of a separate argument
  • When the Framer is creating multiple dbes at once, persist them via a single insert to Mongo;remove our previous hackery and band-aids around this sort of thing

File Changes

  • M app/controllers/v1/roll_controller.rbhttps://github.com/ShelbyTV/shelby_gt/pull/207/files#diff-0(2)
  • M lib/gt/framer.rbhttps://github.com/ShelbyTV/shelby_gt/pull/207/files#diff-1(107)
  • M lib/gt/recommendation_manager.rbhttps://github.com/ShelbyTV/shelby_gt/pull/207/files#diff-2(2)
  • M spec/lib/gt/unit/framer_spec.rbhttps://github.com/ShelbyTV/shelby_gt/pull/207/files#diff-3(408)
  • M spec/lib/gt/unit/recommendation_manager_spec.rbhttps://github.com/ShelbyTV/shelby_gt/pull/207/files#diff-4(2)

Patch Links:

spinosa commented 10 years ago

To make sure this doesn't have any adverse affects on Arnold in production, you should update a single Arnold box (cap will deploy, restart is still manual) and see if it performs at the same levels before this change.

graphite shows breakdown of Arnold performance: http://graphite.shelby.tv/dashboard/#Arnold

spinosa commented 10 years ago

beyond the considerations I listed above, this pull looks good to go