Upgrade to Pro — share decks privately, control downloads, hide ads and more …

Rewriting Critical Parts of Your Application Us...

Rewriting Critical Parts of Your Application Using... Science!

The best advice I ever received about code was to never get attached to it, because eventually it will need to change. Things always change; and a robust system must be able to respond to these changes to survive and grow. You might be able to manage the small changes along the way, but eventually as your application gets older, there will come a day where you're going to have to make a big change -- a change so big that you're not just changing a piece of the system anymore, you're rewriting it.
The scariest thing about a rewrite is that usually the old system is very complicated and its behavior isn't completely documented, so it's hard to be sure that any new system you create replaces the functionality of the old system completely. This might be good enough for certain rewrites -- it might even be a good opportunity to rethink some behavior -- but there are other cases where you can't afford to be unsure about the correctness of the system you're replacing because it's too important.
At GitHub, we found ourselves at a point where we needed to rewrite the system that models our permissions in order to be able to build new features and improve performance. This talk will cover the strategies we developed to rewrite this critical piece of the app side-by-side with the existing code, specifically how we used our open-source science and analysis libraries to rewrite small pieces at a time and ensure that everything worked when we finally flipped the switch.

Jesse Toth

June 05, 2014
Tweet

More Decks by Jesse Toth

Other Decks in Programming

Transcript

  1. !

  2. ! ! separate ways of granting permissions living side-by-side "

    tons of bugs and edge cases around transitional states # performance degradation
  3. !

  4. !

  5. ! +------+ | User |-------------- +------+ +------+ | Team |--------------

    +------+ +------------+ | Repository |-------- +------------+
  6. ! +------+ | User |---------------- +------+ +------+ 1 | Team

    |--------+------- +------+ | | +------------+ v | Repository |---------- +------------+
  7. ! +------+ 2 | User |-----------+---- +------+ | | +------+

    1 v | Team |--------+------- +------+ | | +------------+ v | Repository |---------- +------------+
  8. ! +------+ 2 3 | User |-----------+--+- +------+ | .

    | . +------+ 1 v . | Team |--------+-----.- +------+ | . | . +------------+ v v | Repository |---------- +------------+
  9. ! existing API easy to compare class Repository # Is

    the given user allowed # to pull this repository? def pullable_by?(user) # is the user a collaborator # or a member of a team with access # or part of some other edge case? . . . end end
  10. ! lists of repositories, pull requests, teams, etc. # Find

    all repositories which this organization # controls the access to. ! def organization.controlled_repositories . . . end
  11. ! lists of repositories, pull requests, teams, etc. # Find

    all pull requests to which this user # has access. Access to PRs follow the user’s # access rights to the PR’s repository. ! def user.accessible_pull_requests . . . end
  12. ! SELECT r.id FROM r, (( SELECT r.id as r_ids,

    2 as perms from r WHERE r.owner_id = 99999 AND (r.x = 0) ) UNION ALL ( SELECT r.id as r_ids, 1 as perms from r INNER JOIN p ON r.id = p.r_id WHERE p.u_id = 99999 AND (r.x = 0) ) UNION ALL ( SELECT r.id as r_ids, 2 as perms from r INNER JOIN t ON r.o_id = t.o_id INNER JOIN t_m ON t.id = t_m.t_id WHERE t.name = 'X' AND t_m.u_id = 99999 AND (r.x = 0) ) UNION ALL ( SELECT r.id as r_ids, GROUP_CONCAT(distinct t.p) as perms from r INNER JOIN t_m r_t ON r.id = r_t.r_id INNER JOIN t ON r_t.t_id = t.id INNER JOIN t_m u_t ON t.id = u_t.t_id WHERE u_t.u_id = 99999 AND t.name != 'X' AND t.p in (2, 1, 0) AND (r.x = 0) GROUP BY r.id ) UNION ALL ( SELECT r.id as r_ids, 0 as perms from r JOIN u ON r.plan_owner_id = u.id JOIN t ON t.o_id = u.id JOIN t_m ON t.id = t_m.t_id WHERE u.type = 'XX' AND t.name = 'X' AND t_m.u_id = 99999 AND (r.x = 0) AND r.parent_id IS NOT NULL )) AS unioned WHERE r.id = r_ids;
  13. ! class Repository def pullable_by?(user) science "ability.repository.pullable-by" do |e| e.context

    :user => (user.id if user), :repo => id ! e.control { pullable_by_legacy?(user) } e.candidate { pullable_by_abs?(user) } end end ! def pullable_by_abs?(user) user.can? :read, self end ! def pullable_by_legacy?(user) # original code for pullable_by? # collaborators || team || special case end end
  14. ! class Repository def pullable_by?(user) science "ability.repository.pullable-by" do |e| e.context

    :user => (user.id if user), :repo => id ! e.control { pullable_by_legacy?(user) } e.candidate { pullable_by_abs?(user) } end end ! def pullable_by_abs?(user) user.can? :read, self end ! def pullable_by_legacy?(user) # original code for pullable_by? # collaborators || team || special case end end
  15. ! class Repository def pullable_by?(user) science "ability.repository.pullable-by" do |e| e.context

    :user => (user.id if user), :repo => id ! e.control { pullable_by_legacy?(user) } e.candidate { pullable_by_abs?(user) } end end ! def pullable_by_abs?(user) user.can? :read, self end ! def pullable_by_legacy?(user) # original code for pullable_by? # collaborators || team || special case end end
  16. ! class Repository def pullable_by?(user) science "ability.repository.pullable-by" do |e| e.context

    :user => (user.id if user), :repo => id ! e.control { pullable_by_legacy?(user) } e.candidate { pullable_by_abs?(user) } end end ! def pullable_by_abs?(user) user.can? :read, self end ! def pullable_by_legacy?(user) # original code for pullable_by? # collaborators || team || special case end end
  17. ! class Repository def pullable_by?(user) science "ability.repository.pullable-by" do |e| e.context

    :user => (user.id if user), :repo => id ! e.control { pullable_by_legacy?(user) } e.candidate { pullable_by_abs?(user) } end end ! def pullable_by_abs?(user) user.can? :read, self end ! def pullable_by_legacy?(user) # original code for pullable_by? # collaborators || team || special case end end
  18. ! class Repository def pullable_by?(user) science "ability.repository.pullable-by" do |e| e.context

    :user => (user.id if user), :repo => id ! e.control { pullable_by_legacy?(user) } e.candidate { pullable_by_abs?(user) } end end ! def pullable_by_abs?(user) user.can? :read, self end ! def pullable_by_legacy?(user) # original code for pullable_by? # collaborators || team || special case end end
  19. subscribe /^science\./ do |name, . . ., payload| experiment =

    "science.#{payload[:experiment]}" ! increment "#{experiment}.total" timing "#{experiment}.control", payload[:control][:duration] timing "#{experiment}.candidate", payload[:candidate][:duration] end ! subscribe /^science\.mismatch/ do |name, . . ., payload| experiment = "science.#{payload[:experiment]}" ! increment "#{experiment}.wrong" Redis.lpush "#{experiment}.mismatch", payload.to_json end
  20. subscribe /^science\./ do |name, . . ., payload| experiment =

    "science.#{payload[:experiment]}" ! increment "#{experiment}.total" timing "#{experiment}.control", payload[:control][:duration] timing "#{experiment}.candidate", payload[:candidate][:duration] end ! subscribe /^science\.mismatch/ do |name, . . ., payload| experiment = "science.#{payload[:experiment]}" ! increment "#{experiment}.wrong" Redis.lpush "#{experiment}.mismatch", payload.to_json end
  21. subscribe /^science\./ do |name, . . ., payload| experiment =

    "science.#{payload[:experiment]}" ! increment "#{experiment}.total" timing "#{experiment}.control", payload[:control][:duration] timing "#{experiment}.candidate", payload[:candidate][:duration] end ! subscribe /^science\.mismatch/ do |name, . . ., payload| name = payload[:experiment] experiment = "science.#{name}" ! increment "#{experiment}.wrong" Redis.lpush "#{experiment}.mismatch", payload.to_json end
  22. subscribe /^science\./ do |name, . . ., payload| experiment =

    "science.#{payload[:experiment]}" ! increment "#{experiment}.total" timing "#{experiment}.control", payload[:control][:duration] timing "#{experiment}.candidate", payload[:candidate][:duration] end ! subscribe /^science\.mismatch/ do |name, . . ., payload| experiment = "science.#{payload[:experiment]}" ! increment "#{experiment}.wrong" Redis.lpush "#{experiment}.mismatch", payload.to_json end
  23. subscribe /^science\./ do |name, . . ., payload| experiment =

    "science.#{payload[:experiment]}" ! increment "#{experiment}.total" timing "#{experiment}.control", payload[:control][:duration] timing "#{experiment}.candidate", payload[:candidate][:duration] end ! subscribe /^science\.mismatch/ do |name, . . ., payload| experiment = "science.#{payload[:experiment]}" ! increment "#{experiment}.wrong" Redis.lpush "#{experiment}.mismatch", payload.to_json end
  24. subscribe /^science\./ do |name, . . ., payload| experiment =

    "science.#{payload[:experiment]}" ! increment "#{experiment}.total" timing "#{experiment}.control", payload[:control][:duration] timing "#{experiment}.candidate", payload[:candidate][:duration] end ! subscribe /^science\.mismatch/ do |name, . . ., payload| experiment = "science.#{payload[:experiment]}" ! increment "#{experiment}.wrong" Redis.lpush "#{experiment}.mismatch", payload.to_json end
  25. !

  26. !

  27. ! class Analysis def read Redis.rpop "science.#{experiment_name}.mismatch" end def count

    Redis.llen "science.#{experiment_name}.mismatch" end end
  28. ! irb(main):001> analysis = Analysis.new('ability.repository.pullable-by') => #<Analysis:0x007fd4c7f5c4d8 @experiment_name="ability.repository.pullable-by", @wrappers=[], @registry=#<Analysis::Registry:0x007fd4c7f72b98

    @known_classes=[]>> ! irb(main):002> analysis.count => 3283 ! irb(main):003> result = analysis.fetch => {"from"=>"GitHub::Jobs::ProcessEvent", "experiment"=>"ability.repository.pullable-by", "user"=>XXXXXXX, “repo"=>XXXXXXX, "timestamp"=>"2014-05-28T18:53:16-07:00", "candidate"=>{"duration"=>0.41368900000000003, "exception"=>nil, "value"=>false}, "control"=>{"duration"=>1.141963, "exception"=>nil, "value"=>true}, "first"=>"control"} !
  29. ! irb(main):001> analysis = Analysis.new('ability.repository.pullable-by') => #<Analysis:0x007fd4c7f5c4d8 @experiment_name="ability.repository.pullable-by", @wrappers=[], @registry=#<Analysis::Registry:0x007fd4c7f72b98

    @known_classes=[]>> ! irb(main):002> analysis.count => 3283 ! irb(main):003> result = analysis.fetch => {"from"=>"GitHub::Jobs::ProcessEvent", "experiment"=>"ability.repository.pullable-by", "user"=>XXXXXXX, “repo"=>XXXXXXX, "timestamp"=>"2014-05-28T18:53:16-07:00", "candidate"=>{"duration"=>0.41368900000000003, "exception"=>nil, "value"=>false}, "control"=>{"duration"=>1.141963, "exception"=>nil, "value"=>true}, "first"=>"control"} !
  30. ! irb(main):001> analysis = Analysis.new('ability.repository.pullable-by') => #<Analysis:0x007fd4c7f5c4d8 @experiment_name="ability.repository.pullable-by", @wrappers=[], @registry=#<Analysis::Registry:0x007fd4c7f72b98

    @known_classes=[]>> ! irb(main):002> analysis.count => 3283 ! irb(main):003> result = analysis.fetch => {"from"=>"GitHub::Jobs::ProcessEvent", "experiment"=>"ability.repository.pullable-by", "user"=>XXXXXXX, “repo"=>XXXXXXX, "timestamp"=>"2014-05-28T18:53:16-07:00", "candidate"=>{"duration"=>0.41368900000000003, "exception"=>nil, "value"=>false}, "control"=>{"duration"=>1.141963, "exception"=>nil, "value"=>true}, "first"=>"control"} !
  31. ! irb(main):001> analysis = Analysis.new('ability.repository.pullable-by') => #<Analysis:0x007fd4c7f5c4d8 @experiment_name="ability.repository.pullable-by", @wrappers=[], @registry=#<Analysis::Registry:0x007fd4c7f72b98

    @known_classes=[]>> ! irb(main):002> analysis.count => 3283 ! irb(main):003> result = analysis.fetch => {"from"=>"GitHub::Jobs::ProcessEvent", "experiment"=>"ability.repository.pullable-by", "user"=>XXXXXXX, “repo"=>XXXXXXX, "timestamp"=>"2014-05-28T18:53:16-07:00", "candidate"=>{"duration"=>0.41368900000000003, "exception"=>nil, "value"=>false}, "control"=>{"duration"=>1.141963, "exception"=>nil, "value"=>true}, "first"=>"control"} !
  32. ! irb(main):001> analysis = Analysis.new('ability.repository.pullable-by') => #<Analysis:0x007fd4c7f5c4d8 @experiment_name="ability.repository.pullable-by", @wrappers=[], @registry=#<Analysis::Registry:0x007fd4c7f72b98

    @known_classes=[]>> ! irb(main):002> analysis.count => 3283 ! irb(main):003> result = analysis.fetch => {"from"=>"GitHub::Jobs::ProcessEvent", "experiment"=>"ability.repository.pullable-by", "user"=>XXXXXXX, “repo"=>XXXXXXX, "timestamp"=>"2014-05-28T18:53:16-07:00", "candidate"=>{"duration"=>0.41368900000000003, "exception"=>nil, "value"=>false}, "control"=>{"duration"=>1.141963, "exception"=>nil, "value"=>true}, "first"=>"control"} !
  33. !