adding review guidelines

Change-Id: I61304856a4ecccbbf3aa06c30822494592a3b3d5

1 files changed, 387 insertions, 0 deletions

diff --git a/REVIEW_GUIDELINES.rst b/REVIEW_GUIDELINES.rst
new file mode 100644
index 000000000..fe56c98e7
--- /dev/null
+++ b/REVIEW_GUIDELINES.rst
@@ -0,0 +1,387 @@
+Review Guidelines
+=================
+
+Effective code review is a skill like any other professional skill you
+develop with experience. Effective code review requires trust. No
+one is perfect. Everyone makes mistakes. Trust builds over time.
+
+This document will enumerate behaviors commonly observed and
+associated with competent reviews of changes purposed to the Swift
+code base. No one is expected to "follow these steps". Guidelines
+are not *rules*, not all behaviors will be relevant in all situations.
+
+
+Checkout the Change
+-------------------
+
+You will need to have a copy of the change in an environment where you
+can freely edit and experiment with the code in order to provide a
+non-superficial review. Superficial reviews are not terribly helpful.
+Always try to be helpful. ;)
+
+Check out the change so that you may begin.
+
+Commonly, ``git review -d <change-id>``
+
+Run it
+------
+
+Imagine that you submit a patch to Swift, and a reviewer starts to
+take a look at it. Your commit message on the patch claims that it
+fixes a bug or adds a feature, but as soon as the reviewer downloads
+it locally and tries to test it, a severe and obvious error shows up.
+Something like a syntax error or a missing dependency.
+
+"Did you even run this?" is the review comment all contributors dread.
+
+Reviewers in particular need to be fearful merging changes that just
+don't work - or at least fail in frequently common enough scenarios to
+be considered "horribly broken". A comment in our review that says
+roughly "I ran this on my machine and observed ``description of
+behavior change is supposed to achieve``" is the most powerful defense
+we have against the terrible terrible scorn from our fellow Swift
+developers and operators when we accidentally merge bad code.
+
+If you're doing a fair amount of reviews - you will participate in
+merging a change that will break my clusters - it's cool - I'll do it
+to you at some point too (sorry about that). But when either of us go
+look at the reviews to understand the process gap that allowed this to
+happen - it better not be just because we were too lazy to check it out
+and run it before it got merged.
+
+Or be warned, you may receive, the dreaded...
+
+    "Did you even *run* this?"
+
+I'm sorry, I know it's rough. ;)
+
+Consider edge cases very seriously
+----------------------------------
+
+    Saying "that should rarely happen" is the same as saying "that
+    *will* happen"
+
+    -- Douglas Crockford
+
+Scale is an *amazingly* abusive partner. If you contribute changes to
+Swift your code is running - in production - at scale - and your bugs
+can not hide. I wish on all of us that our bugs may be exceptionally
+rare - meaning they only happen in extremely unlikely edge cases. For
+example, bad things that happen only 1 out of every 10K times an op is
+performed will be discovered in minutes. Bad things that happen only
+1 out of every one billion times something happens will be observed -
+by multiple deployments - over the course of a release. Bad things
+that happen 1/100 times some op is performed are considered "horribly
+broken". Tests must exhaustively exercise possible scenarios. Every
+system call and network connection will raise an error and timeout -
+where will that Exception be caught?
+
+Run the tests
+-------------
+
+Yes, I know Gerrit does this already. You can do it *too*. You might
+not need to re-run *all* the tests on your machine - it depends on the
+change. But, if you're not sure which will be most useful - running
+all of them best - unit - functional - probe. If you can't reliably
+get all tests passing in your development environment you will not be
+able to do effective reviews. Whatever tests/suites you are able to
+exercise/validate on your machine against your config you should
+mention in your review comments so that other reviewers might choose
+to do *other* testing locally when they have the change checked out.
+
+e.g.
+
+    I went ahead and ran probe/test_object_metadata_replication.py on
+    my machine with both sync_method = rsync and sync_method = ssync -
+    that works for me - but I didn't try it with object_post_as_copy =
+    false
+
+Maintainable Code is Obvious
+----------------------------
+
+Style is an important component to review. The goal is maintainability.
+
+However, keep in mind that generally style, readability and
+maintainability are orthogonal to the suitability of a change for
+merge. A critical bug fix may be a well written pythonic masterpiece
+of style - or it may be a hack-y ugly mess that will absolutely need
+to be cleaned up at some point - but it absolutely should merge
+because: CRITICAL. BUG. FIX.
+
+You should comment inline to praise code that is "obvious". You should
+comment inline to highlight code that that you found to be "obfuscated".
+
+Unfortunately "readability" is often subjective. We should remember
+that it's probably just our own personal preference. Rather than a
+comment that says "You should use a list comprehension here" - rewrite
+the code as a list comprehension, run the specific tests that hit the
+relevant section to validate your code is correct, then leave a
+comment that says:
+
+    I find this more readable:
+
+    ``diff with working tested code``
+
+If the author (or another reviewer) agrees - it's possible the change will get
+updated to include that improvement before it is merged; or it may happen in a
+follow up.
+
+However, remember that style is non-material - it is useful to provide (via
+diff) suggestions to improve maintainability as part of your review - but if
+the suggestion is functionally equivalent - it is by definition optional.
+
+Commit Messages
+---------------
+
+Read the commit message thoroughly before you begin the review.
+
+Commit messages must answer the "why" and the "what for" - more so
+than the "how" or "what it does". Commonly this will take the form of
+a short description:
+
+- What is broken - without this change
+- What is impossible to do with Swift - without this change
+- What is slower/worse/harder - without this change
+
+If you're not able to discern why a change is being made or how it
+would be used - you may have to ask for more details before you can
+successfully review it.
+
+Commit messages need to have a high consistent quality. While many
+things under source control can be fixed and improved in a follow up
+change - commit messages are forever. Luckily it's easy to fix minor
+mistakes using the in-line edit feature in Gerrit!  If you can avoid
+ever having to *ask* someone to change a commit message you will find
+yourself an amazingly happier and more productive reviewer.
+
+Also commit messages should follow the OpenStack Commit Message
+guidelines, including references to relevant impact tags or bug
+numbers. You should hand out links to the OpenStack Commit Message
+guidelines *liberally* via comments when fixing commit messages during
+review.
+
+Here you go: `GitCommitMessages <https://wiki.openstack.org/wiki/GitCommitMessages#Summary_of_Git_commit_message_structure>`_
+
+New Tests
+---------
+
+New tests should be added for all code changes. Historically you
+should expect good changes to have a diff line count ratio of at least
+2:1 tests to code. Even if a change has to "fix" a lot of *existing*
+tests, if a change does not include any *new* tests it probably should
+not merge.
+
+If a change includes a good ratio of test changes and adds new tests -
+you should say so in your review comments.
+
+If it does not - you should write some!
+
+... and offer them to the patch author as a diff indicating to them that
+"something" like these tests I'm providing as an example will *need* to be
+included in this change before it is suitable to merge. Bonus points if you
+include suggestions for the author as to how they might improve or expanded on
+the tests stubs you provide.
+
+Be *very* careful about asking an author to add a test for a "small change"
+before attempting to do so yourself. It's quite possible there is a lack of
+existing test infrastructure needed to develop a concise and clear test - the
+author of a small change may not be the best person to introduce a large
+amount of new test infrastructure. Also, most of the time remember it's
+*harder* to write the test than the change - if the author is unable to
+develop a test for their change on their own you may prevent a useful change
+from being merged. At a minimum you should suggest a specific unit test that
+you think they should be able to copy and modify to exercise the behavior in
+their change. If you're not sure if such a test exists - replace their change
+with an Exception and run tests until you find one that blows up.
+
+Documentation
+-------------
+
+Most changes should include documentation. New functions and code
+should have Docstrings. Tests should obviate new or changed behaviors
+with descriptive and meaningful phrases. New features should include
+changes to the documentation tree. New config options should be
+documented in example configs. The commit message should document the
+change for the change log.
+
+Always point out typos or grammar mistakes when you see them in
+review, but also consider that if you were able to recognize the
+intent of the statement - documentation with tpyos may be easier to
+iterate and improve on than nothing.
+
+If a change does not have adequate documentation it may not be suitable to
+merge. If a change includes incorrect or misleading documentation or is
+contrary to *existing* documentation is probably is not suitable to merge.
+
+Every change could have better documentation.
+
+Like with tests, a patch isn't done until it has docs. Any patch that
+adds a new feature, changes behavior, updates configs, or in any other
+way is different than previous behavior requires docs. manpages,
+sample configs, docstrings, descriptive prose in the source tree
+
+Reviewers Write Code
+--------------------
+
+Reviews have been shown to to provide many benefits - one of which is shared
+ownership. After providing a positive review you should understand how the
+change works. Doing this will probably require you to "play with" the change.
+
+You might functionally test the change in various scenarios. You may need to
+write a new unittest to validate the change will degrade gracefully under
+failure. You might have to write a script to exercise the change under some
+superficial load. You might have to break the change and validate the new
+tests fail and provide useful errors. You might have to step through some
+critical section of the code in a debugger to understand when all the possible
+branches are exercised in tests.
+
+When you're done with your review an artifact of your effort will be
+observable in the piles of code and scripts and diffs you wrote while
+reviewing. You should make sure to capture those artifacts in a paste
+or gist and include them in your review comments so that others may
+reference them.
+
+e.g.
+
+    When I broke the change like this:
+
+    ``diff``
+
+    it blew up like this:
+
+    ``unittest failure``
+
+
+It's not uncommon that a review takes more time than writing a change -
+hopefully the author also spent as much time as you did *validating* their
+change but that's not really in your control. When you provide a positive
+review you should be sure you understand the change - even seemingly trivial
+changes will take time to consider the ramifications.
+
+Leave Comments
+--------------
+
+Leave. Lots. Of. Comments.
+
+A popular web comic has stated that
+`WTFs/Minute <http://www.osnews.com/images/comics/wtfm.jpg>`_ is the
+*only* valid measurement of code quality.
+
+If something initially strikes you as questionable - you should jot
+down a note so you can loop back around to it.
+
+However, because of the distributed nature of authors and reviewers
+it's *imperative* that you try your best to answer your own questions
+as part of your review.
+
+Do not say "Does this blow up if it gets called when xyz" - rather try
+and find a test that specifically covers that condition and mention it
+in the comment so others can find it more quickly. Of if you can find
+no such test, add one to demonstrate the failure, and include a diff
+in a comment. Hopefully you can say "I *thought* this would blow up,
+so I wrote this test, but it seems fine."
+
+But if your initial reaction is "I don't understand this" or "How does
+this even work?" you should notate it and explain whatever you *were*
+able to figure out in order to help subsequent reviewers more quickly
+identify and grok the subtle or complex issues.
+
+Because you will be leaving lots of comments - many of which are
+potentially not highlighting anything specific - it is VERY important
+to leave a good summary. Your summary should include details of how
+you reviewed the change. You may include what you liked most, or
+least.
+
+If you are leaving a negative score ideally you should provide clear
+instructions on how the change could be modified such that it would be
+suitable for merge - again diffs work best.
+
+Scoring
+-------
+
+Scoring is subjective. Try to realize you're making a judgment call.
+
+A positive score means you believe Swift would be undeniably better
+off with this code merged than it would be going one more second
+without this change running in production immediately. It is indeed
+high praise - you should be sure.
+
+A negative score means that to the best of your abilities you have not
+been able to your satisfaction, to justify the value of a change
+against the cost of it's deficiencies and risks. It is a surprisingly
+difficult chore to be confident about the value of unproven code or a
+not well understood use-case in an uncertain world, and unfortunately
+all too easy with a **thorough** review to uncover our defects, and be
+reminded of the risk of... regression.
+
+Reviewers must try *very* hard first and foremost to keep master stable.
+
+If you can demonstrate a change has an incorrect *behavior* it's
+almost without exception that the change must be revised to fix the
+defect *before* merging rather than letting it in and having to also
+file a bug.
+
+Every commit must be deployable to production.
+
+Beyond that - almost any change might be merge-able depending on
+it's merits!  Here's some tips you might be able to use to find more
+changes that should merge!
+
+#. Fixing bugs is HUGELY valuable - the *only* thing which has a
+   higher cost than the value of fixing a bug - is adding a new
+   bug - if it's broken and this change makes it fixed (with out
+   breaking anything else) you have a winner!
+
+#. Features are INCREDIBLY difficult to justify their value against
+   the cost of increased complexity, lowered maintainability, risk
+   of regression, or new defects. Try to focus on what is
+   *impossible* without the feature - when you make the impossible
+   possible things are better. Make things better.
+
+#. Purely test/doc changes, complex refactoring, or mechanical
+   cleanups are quite nuanced because there's less concrete
+   objective value. I've seen lots of these kind of changes
+   get lost to the backlog. I've also seen some success where
+   multiple authors have collaborated to "push-over" a change
+   rather than provide a "review" ultimately resulting in a
+   quorum of three or more "authors" who all agree there is a lot
+   of value in the change - however subjective.
+
+Because the bar is high - most reviews will end with a negative score.
+
+However, for non-material grievances (nits) - you should feel
+confident in a positive review if the change is otherwise complete
+correct and undeniably makes Swift better (not perfect, *better*). If
+you see something worth fixing you should point it out in review
+comments, but when applying a score consider if it *need* be fixed
+before the change is suitable to merge vs. fixing it in a follow up
+change?  Consider if the change makes Swift so undeniably *better*
+and it was deployed in production without making any additional
+changes would it still be correct and complete?  Would releasing the
+change to production without any additional follow up make it more
+difficult to maintain and continue to improve Swift?
+
+Endeavor to leave a positive or negative score on every change you review.
+
+Use your best judgment.
+
+A note on Swift Core Maintainers
+================================
+
+Swift Core maintainers may provide positive reviews scores that *look*
+different from your reviews - a "+2" instead of a "+1"
+
+But it's *exactly the same* as your "+1"
+
+It means the change has been thoroughly and positively reviewed. The
+only reason it's different is to help identify changes which have
+received multiple competent and positive reviews. If you consistently
+provide competent reviews you run a *VERY* high risk of being
+approached to have your future positive review scores changed from a
+"+1" to "+2" in order to make it easier to identify changes which need
+to get merged.
+
+Ideally a review from a core maintainer should provide a clear path
+forward for the patch author. If you don't know how to proceed
+respond to the reviewers comments on the change and ask for help.
+We'd love to try and help.