jwinter.org

A place for everything and everything in its place

Code Reviews @jwinter-style

I really enjoy reviewing code. I think it’s mostly because I enjoy listening and talking to people about programming. So here are some of my rambling thoughts on reviewing code.

First, I try to be cognizant of other people’s feelings during code review. It’s easy to be rude by giving curt, negative feedback. Sometimes it takes more time to reword a comment in more sensitive language. I’ve never regretted taking the time to do that. I have regretted hastily sending off a comment that sounds like a jerk wrote it.

Speaking of which, there have been times where following my feedback would have absolutely resulted in a change for the worse. If you review code long enough and have a modicum of self-awareness, this will also happen to you. Be prepared for it and let the author and other reviewers know when you change your mind.

I also try to respect the author of the pull request by doing the following (and sometimes I screw this up):

  1. Point out what they’ve done well! Everybody forgets this one! Why don’t you do this?!
  2. Phrase refactoring suggestions as suggestions. Rarely, I “very strongly” suggest a refactoring, but even in that case I try to say so in a way that respects the work that’s already gone into this change.
  3. Ask before I add commits to their pull request. If I don’t get a response quickly, I’ll generally just put some sample code into my comments to show what I’m suggesting.
  4. Provide performance specifics. When I think the code is too slow, instead of just saying something like “This looks too slow”, I’ll try to provide specific numbers or offer help in profiling.
  5. OFFER HELP! I’ll offer to pair on something during or after a review.
  6. When I do think the code needs reworking, I either provide a specific way to improve the code or have one in mind. I won’t always offer the change I have in mind because that sometimes narrows the author’s thinking on how to make an improvement.

So here are the actual steps I go through.

First, I check out the branch to review in my local git repo. If you don’t check out the branch you’re reviewing locally, I’m not really sure how you can provide a close review. There’s several important steps below that you just can’t do by looking at the code on Github alone.

Next, if the branch won’t merge cleanly, I’ll rebase it locally (and not push my changes) before starting this process. If there are a lot of merge conflicts, I’ll ask for the author to merge master into this branch or rebase it on master, or whatever they prefer to get it closer to our master branch.

Then, I run our entire test suite (unit, functional, integration, whatever) locally. Some may be wondering why I just don’t just let our continuous integration server run the tests on the branch. I run tests locally, in addition to the CI server, because once this code passes review, it will be the new master branch. And I will be using that immediately in whatever I’m working on. So if my local dev environment is going to start failing tests, I want to know as soon as possible. It’s also a great time to ask for help because the person sending the pull request still has the code fresh in their mind and is especially motivated to get the change through.

While the tests are running, I read the diffs on Github. I like to look at the whole diff across all files. If it’s a particularly large or complicated pull request, I may drill into specific commits. But I don’t really care which commit a particular change came from, since I’m not reviewing that commit I’m reviewing the whole branch.

Some quick things I look for on my first pass:

  1. Any change where there’s not a corresponding change in test code
  2. New methods or variables that go unused
  3. Anything that doesn’t match our (implicit or explict) style guide
  4. Anything that reminds me of a bug I’ve hit before. An example here might be using ||= for memoization in Ruby
  5. Any goddamn trailing whitespace
  6. Anything that I want to take a closer look at

Things that fall under that last bullet would be any change in data structure, any new data structures, any change in logic or addition of new logic, etc. Basically anything other than the simplest 1-2 line change where only I could screw it up, I’ll want to take a closer look at.

After that first pass, I’ll either be just about done if it’s a small change or ready to start taking a closer look at the bits I’ve identified from the first pass.

This is the point where I usually read the request that spawned this change (bug report/feature request/bar napkin) and then actually use the new feature or attempt to repro the bug. I don’t think most people do this. This is a really important step, maybe the most important, so please actually run the code that you’re reviewing.

Now, when taking a closer look, I’ll do any combination of the following:

  1. Think about how the change will run in production.
  2. Reverse their logic and make sure the right tests fail.
  3. Drop into the debugger and walk through the code to see if it runs how I expect.
  4. Copy their code into a REPL and make sure it does what I think it’s supposed to.
  5. Walk back up call stack to see if any of the arguments expected to be one type are actually of that type, e.g. non-null, array, etc.
  6. Write a (or run an existing) load or stress test.

And before I finally +1 the change I check that:

  1. Every comment I’ve made has been responded to, either with code or with a discussion.
  2. All tests pass locally.
  3. Any new ideas or features that have come out of review are tracked somewhere.
  4. It still merges cleanly. I’ve seen enough bugs introduced during merge resolution that I’ll ask folks to merge master or rebase before completing my review.

So what’s above is what I use to help me improve the code I look at. I’m very interested in what helps others do the same. Get at me at @jwinter on twitter if you’ve got feedback on this. I’d love to hear it.

Docker Resources

More background on Docker: http://www.docker.io/the-whole-story/ Their very cool Interactive Tutorial: https://www.docker.io/gettingstarted/ Installation via vagrant: http://docs.docker.io/en/latest/installation/vagrant/ dotCloud (creators of Docker) had a series of blog posts on building a PaaS that included ones on LXC and AUFS: http://blog.dotcloud.com/under-the-hood-linux-kernels-on-dotcloud-part http://blog.dotcloud.com/kernel-secrets-from-the-paas-garage-part-34-a

A Short Talk on Docker

I gave a short talk on Docker at PhillyRB’s August meeting. These were my slides, although I went way off of them and only got through about half. What follows are the notes I used for my talk.

What’s Docker?

3 things I’d like you to take from this talk:

  • Docker provides a lot of the advantages of virtual machines without the disadvantages
  • Container-based deployment is a great way to deploy applications
  • AUFS and LXC are the chocolate and peanut butter that makes Docker so great to use

What’s a container?

It’s like a virtual machine. When your app is running inside a container, it appears to be running inside its own OS. But there could be many other containers running alongside yours, all sharing resources. It uses a combination of LXC (Linux containers) and AUFS (A union/layered/snapshotting filesystem). I’ll talk about both more later

They’re also a means of packaging. You can build a container and run it anywhere that can run docker. You can share that container with other people and know that it will run the same way.

Why are they useful?

  • Run your whole damn stack
    Run your Rails app on your laptop by grabbing a container with RVM installed, another container with PostgreSQL, another container with Redis, another with ElasticSearch or Solr. If you’ve ever tried to run multi-node Vagrant, it’s really painful and slow to spin up several nodes. Docker makes it very fast and easy.
  • Reduce dependencies by shipping dependencies inside the container
    Compile native gems or anything else once inside your container and ship that everywhere instead of building on each server.
  • Cut down on configuration errors in production
    Configure your OS inside the container then snapshot.
  • They’re fast
    Much faster and less resource intensive than VMs. As we’ll see in the demo, you can spin one up almost instantly.
  • Deploy Once, Deploy Anywhere
    Once you’ve built a container, and committed it as an image you can run it anywhere
  • Let someone else do the work for you
    The ability to package and share containers means that you can use someone else’s work to start from a known functional state. It’s like sharing chef cookbooks.
  • Easy to build
    Make a mistake building your application, no problem, every successful step is automatically snapshotted. If you’ve ever tried provisioning VMs with vagrant you know you’re almost always starting over every time.

What does working with containers look like?

Demo goes here.

How do containers work?

LXC

  • LXC gives you isolation and control over sharing of resources
  • LXC namespaces are what makes each container appear to have its own networking, process table, filesystem mount points, and hostname
  • LXC is also how you say: “Give my Rails container 4GB of RAM, but my database container 28GB”. It gives you some control over resource management including IO ops/second and bytes per second.

AUFS

  • AUFS, union or “layering” filesystem
  • The filesystem layers give each container its own view of the complete filesystem
  • Base layer is the OS and is read-only
  • Your container is read-write on top
  • When you go to do a read, it hits your container first, then reads through to the base if you haven’t made any changes
  • Unchanged files are shared
  • Buffercache is also shared, so a certain portion of what’s shared on disk is shared in memory as well.
  • This aspect is importants because it improves performance. Storage is cheap, so who really cares that AUFS saves a few gigabytes, but the fact that many dynamic libraries are shared across containers in memory means that containers spin up very quickly.

Where can I find useful prebuilt containers?

  • https://index.docker.io/
  • Search Github for Dockerfiles

How do I build containers?

  • Dockerfile, like a Gemfile but more like a shell script
  • You list what image you want to start from, then you list the commands you want to run, files you want to include, ports you want to expose

What are the downsides?

  • It’s very new, but doesn’t feel buggy. There’s just not a lot of good answers yet for common ops questions like what happens a container crashes or the daemon crashes or how to monitor?
  • Have to run your own repository to share private docker images

What’s the future?

  • Already several platforms like Heroku are popping up to support running Docker containers
  • Docker 1.0 in the fall
  • Like 100 committers on it and they just opensourced it
  • Docker is already changing how people ship software
  • Do MUCH less configuration in Chef, ship almost entirely configured containers
  • Test Chef recipes very quickly with test-kitchen-docker
  • Continuous Integration, test app inside lightweight container, ship that container

Advice for Personal Projects

http://movieos.org/blog/2013/rules-for-personal-projects/

This makes a lot of sense, I’m often lost when I try to go back to a personal project that I’ve left off for a few weeks. Something like a “run” bash script that just did “rails s && open ‘http://localhost:3000/last_thing_i_worked_on’” would be good to get started again quickly. And taking the time to deploy the little Clojure projects I’ve done to heroku would be worthwhile.

Setting Up a New MacBook Pro for Ruby / RoR Development

Install Xcode _or_ just the Xcode command line utitilies (I installed all of Xcode)

 

Pivotal Labs: soloist gem, pivotal_workstation

errored out on SizeUp

errored out on MacVim

errored out on gem_no_ri_no_rdoc

 

Mouse Locator defaults to on and captures Ctrl-space. If you’re an emacs user, you’ll know why this is a problem.

 

Installing ruby-1.8.7 `env CC=/usr/bin/gcc rvm install ruby-1.8.7`

 

Things I installed manually, but would like to automate:

iTerm2 preference for swapping Cmd and Option AND making “Option” send Esc-+ (meta)

emacs 24 - look at the emacs24 recipe

Installed emacs from emacs 24 dmg

oh-my-zsh

 

EventMachine Timers

There is a limit on the number of running eventmachine timers you can use at one time.  And cancelling a timer does *not* free up one of those slots for running timers, you must wait until the time when the timer would actually fire to free up a slot. The default on most systems is 1000 timers. set_max_timers(count) allows you to increase it.

This doesn’t apply to the pure Ruby eventmachine, it has no max_timer limit.

Emacs, Ansi-term, and Screen

C-c C-j turns your ansi-term buffer into a normal emacs buffer where you can copy/paste. C-c C-k switches it back.

So, for example, if you want to paste text into your ansi-term session, you’d copy that text from your emacs buffer, switch to your ansi-term buffer, hit C-c C-j, then M-y, then C-c C-k.

If you use emacs and you’re not using ansi-term and screen, you should give it a shot.