| <!DOCTYPE html> |
| <html> |
| <head> |
| <meta charset="utf-8"> |
| <title>Apache Mesos - Effective Code Reviewing</title> |
| <meta name="viewport" content="width=device-width, initial-scale=1.0"> |
| |
| <meta property="og:locale" content="en_US"/> |
| <meta property="og:type" content="website"/> |
| <meta property="og:title" content="Apache Mesos"/> |
| <meta property="og:site_name" content="Apache Mesos"/> |
| <meta property="og:url" content="http://mesos.apache.org/"/> |
| <meta property="og:image" content="http://mesos.apache.org/assets/img/mesos_logo_fb_preview.png"/> |
| <meta property="og:description" |
| content="Apache Mesos abstracts resources away from machines, |
| enabling fault-tolerant and elastic distributed systems |
| to easily be built and run effectively."/> |
| |
| <meta name="twitter:card" content="summary"/> |
| <meta name="twitter:site" content="@ApacheMesos"/> |
| <meta name="twitter:title" content="Apache Mesos"/> |
| <meta name="twitter:image" content="http://mesos.apache.org/assets/img/mesos_logo_fb_preview.png"/> |
| <meta name="twitter:description" |
| content="Apache Mesos abstracts resources away from machines, |
| enabling fault-tolerant and elastic distributed systems |
| to easily be built and run effectively."/> |
| |
| <link href="//netdna.bootstrapcdn.com/bootstrap/3.1.1/css/bootstrap.min.css" rel="stylesheet"> |
| <link rel="alternate" type="application/atom+xml" title="Apache Mesos Blog" href="/blog/feed.xml"> |
| <link href="../../../assets/css/main.css" media="screen" rel="stylesheet" type="text/css" /> |
| |
| |
| |
| <!-- Google Analytics Magic --> |
| <script type="text/javascript"> |
| var _gaq = _gaq || []; |
| _gaq.push(['_setAccount', 'UA-20226872-1']); |
| _gaq.push(['_setDomainName', 'apache.org']); |
| _gaq.push(['_trackPageview']); |
| |
| (function() { |
| var ga = document.createElement('script'); ga.type = 'text/javascript'; ga.async = true; |
| ga.src = ('https:' == document.location.protocol ? 'https://ssl' : 'http://www') + '.google-analytics.com/ga.js'; |
| var s = document.getElementsByTagName('script')[0]; s.parentNode.insertBefore(ga, s); |
| })(); |
| </script> |
| |
| </head> |
| <body> |
| <!-- magical breadcrumbs --> |
| <div class="topnav"> |
| <div class="container"> |
| <ul class="breadcrumb"> |
| <li> |
| <div class="dropdown"> |
| <a data-toggle="dropdown" href="#">Apache Software Foundation <span class="caret"></span></a> |
| <ul class="dropdown-menu" role="menu" aria-labelledby="dLabel"> |
| <li><a href="http://www.apache.org">Apache Homepage</a></li> |
| <li><a href="http://www.apache.org/licenses/">License</a></li> |
| <li><a href="http://www.apache.org/foundation/sponsorship.html">Sponsorship</a></li> |
| <li><a href="http://www.apache.org/foundation/thanks.html">Thanks</a></li> |
| <li><a href="http://www.apache.org/security/">Security</a></li> |
| </ul> |
| </div> |
| </li> |
| |
| <li><a href="http://mesos.apache.org">Apache Mesos</a></li> |
| |
| |
| <li><a href="/documentation |
| /">Documentation |
| </a></li> |
| |
| |
| </ul><!-- /.breadcrumb --> |
| </div><!-- /.container --> |
| </div><!-- /.topnav --> |
| |
| <!-- navbar excitement --> |
| <div class="navbar navbar-default navbar-static-top" role="navigation"> |
| <div class="container"> |
| <div class="navbar-header"> |
| <button type="button" class="navbar-toggle collapsed" data-toggle="collapse" data-target="#mesos-menu" aria-expanded="false"> |
| <span class="sr-only">Toggle navigation</span> |
| <span class="icon-bar"></span> |
| <span class="icon-bar"></span> |
| <span class="icon-bar"></span> |
| </button> |
| <a class="navbar-brand" href="/"><img src="/assets/img/mesos_logo.png" alt="Apache Mesos logo"/></a> |
| </div><!-- /.navbar-header --> |
| |
| <div class="navbar-collapse collapse" id="mesos-menu"> |
| <ul class="nav navbar-nav navbar-right"> |
| <li><a href="/gettingstarted/">Getting Started</a></li> |
| <li><a href="/blog/">Blog</a></li> |
| <li><a href="/documentation/latest/">Documentation</a></li> |
| <li><a href="/downloads/">Downloads</a></li> |
| <li><a href="/community/">Community</a></li> |
| </ul> |
| </div><!-- /#mesos-menu --> |
| </div><!-- /.container --> |
| </div><!-- /.navbar --> |
| |
| <div class="content"> |
| <div class="container"> |
| <div class="row-fluid"> |
| <div class="col-md-4"> |
| <h4>If you're new to Mesos</h4> |
| <p>See the <a href="/gettingstarted/">getting started</a> page for more |
| information about downloading, building, and deploying Mesos.</p> |
| |
| <h4>If you'd like to get involved or you're looking for support</h4> |
| <p>See our <a href="/community/">community</a> page for more details.</p> |
| </div> |
| <div class="col-md-8"> |
| <h1>Effective Code Reviewing</h1> |
| |
| <h3>Preparing for Review</h3> |
| |
| <p>We’ve found that clear, clean, small, independent, incremental changes are much |
| easier to review thoroughly, and much easier to get committed. Here are some tips |
| to consider before sending review requests:</p> |
| |
| <ol> |
| <li><strong>Use up front discussion</strong>: Surprising a reviewer is likely to lead to |
| wasted effort if the overall design needs changing. Try to get a reviewer |
| on the same page before sending them a review request.</li> |
| <li><strong>Think about how to break up your patch</strong>: Did you make any changes |
| that could be reviewed in independent patches? We use |
| <code>support/post-reviews.py</code>, which makes it easy to create “chains” of |
| reviews based on commits. Become familiar with interactive rebasing |
| (<code>git rebase -i</code>) to split up commits.</li> |
| <li><strong>Provide context for the change</strong>: Make the motivations for the |
| change clear in the review request, so the reviewer is not left |
| guessing. It is highly recommended to attach a JIRA issue with your |
| review for additional context.</li> |
| <li><strong>Follow the <a href="/documentation/latest/./c++-style-guide/">style guide</a> |
| and the style of code around you</strong>.</li> |
| <li><strong>Do a self-review of your changes before publishing</strong>: Approach it |
| from the perspective of a reviewer with no context. Is it easy to figure |
| out the motivations for the change? Does it fit in with the rest of the |
| code style? Are there unnecessary changes that crept in? Did you do any |
| testing of the change?</li> |
| </ol> |
| |
| |
| <h3>Reviewing</h3> |
| |
| <ol> |
| <li><strong>Do high level reviewing before low-level reviewing</strong>: Consider the |
| big picture before you get into any low-level details in the code. |
| For example, do you understand what motivated the change? Why the |
| author chose the particular solution? Does it “feel” more complicated |
| than necessary? If so, invest the time to consider if there are |
| simpler approaches to the problem, or whether the scope of the work |
| can be reduced. These should all be addressed before doing a |
| line-by-line thorough code review, as the latter is subject to a |
| lot of review back-and-forth in the face of changes to the overall |
| approach.</li> |
| <li><strong>Be patient, thoughtful, and respectful</strong>: when providing (a) feedback |
| on reviews and (b) commenting on feedback. Practice |
| <a href="http://blog.codinghorror.com/the-ten-commandments-of-egoless-programming/">ego-less programming</a>, |
| this is not meant to be a sparring match! A prerequisite to being a good |
| reviewee is acknowledging that ‘writing is hard’! The reviewee should give |
| the reviewer the benefit of the doubt that the reviewer has a real concern |
| even if they can’t articulate it well. And the reviewee should be prepared |
| to anticipate all of the reviewers concerns and be thoughtful about why |
| they decided to do something a particular way (especially if it deviates |
| from a standard in the codebase). On the flip side, the reviewer should |
| not use the fact that ‘writing is hard’ as a crutch for giving hasty |
| feedback. The reviewer should invest time and energy trying to explain |
| their concerns clearly and carefully. It’s a two-way street!</li> |
| <li><strong>Resolve issues before committing</strong>: we tend to give a ‘ship it’ even when |
| we think there are some issues that need correcting. Sometimes a particular |
| issue will require more discussion and sometimes we take that discussion to |
| IM or IRC or emails to expedite the process. It’s important, however, that |
| we publicly capture any collaborations/discussions that happened in those |
| means. Making the discussion public also helps involve others that would |
| have liked to get involved but weren’t invited. |
| |
| <ol type="a"> |
| <li>If the reviewer and reviewee are having problems resolving a particular |
| “confrontational” issue then both parties should consider communicating |
| directly or asking another reviewer to participate. <strong>We’re all here to |
| build the highest quality code possible, and we should leverage one |
| another to do so.</strong></li> |
| <li>When an issue is “Dropped” by the reviewee, the expectation is that there |
| must be a reply to the issue indicating why it was dropped. A silent “Drop” |
| is discouraged.</li> |
| <li>If an issue is marked as “Resolved”, the expectation is that the diff has |
| been updated in accordance (more or less) with the reviewer’s comment. If |
| there are significant changes, a reply to the issue with a comment is |
| greatly appreciated.</li> |
| </ol> |
| </li> |
| <li><strong>Be explicit about asking for more feedback</strong>: feel free to update reviews |
| as often as you like but recognize that in many cases it’s ambiguous for |
| reviewers when a review is back into a “ready” state so the reviewee should |
| feel free to ping the reviewers via email when they’re ready.</li> |
| <li><strong>For reviewees, wait for a ‘ship it’</strong>: We often use the original developer |
| or current “custodian” of some particular code to be the reviewer and add others |
| to get more feedback or as an FYI. Feel free to explicitly call out that you’re |
| adding some people just as an FYI in the ‘Description’. It’s worth mentioning |
| that we often reach out directly to one another about a particular change/feature |
| before we even start coding. A little context goes a long way to streamlining the |
| review process.</li> |
| <li><strong>For reviewers, be thorough when giving a ‘ship it’</strong>: understand that |
| reviewing requires a substantial investment of time and focus: |
| |
| <ol type="a"> |
| <li>You are expected to understand and support code that you’re giving a ‘ship it’ to.</li> |
| <li>You are expected to be accountable for the quality of the code you’ve given a ‘ship it’ to.</li> |
| </ol> |
| </li> |
| </ol> |
| |
| |
| </div> |
| </div> |
| |
| </div><!-- /.container --> |
| </div><!-- /.content --> |
| |
| <hr> |
| |
| |
| |
| <!-- footer --> |
| <div class="footer"> |
| <div class="container"> |
| <div class="col-md-4 social-blk"> |
| <span class="social"> |
| <a href="https://twitter.com/ApacheMesos" |
| class="twitter-follow-button" |
| data-show-count="false" data-size="large">Follow @ApacheMesos</a> |
| <script>!function(d,s,id){var js,fjs=d.getElementsByTagName(s)[0],p=/^http:/.test(d.location)?'http':'https';if(!d.getElementById(id)){js=d.createElement(s);js.id=id;js.src=p+'://platform.twitter.com/widgets.js';fjs.parentNode.insertBefore(js,fjs);}}(document, 'script', 'twitter-wjs');</script> |
| <a href="https://twitter.com/intent/tweet?button_hashtag=mesos" |
| class="twitter-hashtag-button" |
| data-size="large" |
| data-related="ApacheMesos">Tweet #mesos</a> |
| <script>!function(d,s,id){var js,fjs=d.getElementsByTagName(s)[0],p=/^http:/.test(d.location)?'http':'https';if(!d.getElementById(id)){js=d.createElement(s);js.id=id;js.src=p+'://platform.twitter.com/widgets.js';fjs.parentNode.insertBefore(js,fjs);}}(document, 'script', 'twitter-wjs');</script> |
| </span> |
| </div> |
| |
| <div class="col-md-8 trademark"> |
| <p>© 2012-2017 <a href="http://apache.org">The Apache Software Foundation</a>. |
| Apache Mesos, the Apache feather logo, and the Apache Mesos project logo are trademarks of The Apache Software Foundation. |
| <p> |
| </div> |
| </div><!-- /.container --> |
| </div><!-- /.footer --> |
| |
| <!-- JS --> |
| <script src="//code.jquery.com/jquery-1.11.0.min.js" type="text/javascript"></script> |
| <script src="//netdna.bootstrapcdn.com/bootstrap/3.1.1/js/bootstrap.min.js" type="text/javascript"></script> |
| </body> |
| </html> |
