Page History

...

Sqoop uses the Apache Review Board for doing code reviews. In order for a change to be reviewed, it should be either posted on the review board or attached to the JIRA. If the change is a minor change affecting only few lines and does not seem to impact main logic of the affected sources, it need not be posted on the review board. However, if the code change is large or otherwise impacting the core logic of the affected sources, it should be posted on the review board. Feel free to comment on the JIRA requesting the assignee to post the patch for review on review board.

Goals for Code Reviews

The net outcome from the review should be the same - which is to ensure the following:

Note: Not all patches attached to a JIRA are ready for review. Sometimes the patches are attached just to solicit early feedback regarding the implementation direction. Feel free to look it over and give your feedback in the JIRA as necessary. Patches are considered ready for review either when the patch has been posted on review board, or the JIRA status has been changed to 'Patch Available'.

Goals for Code Reviews

The net outcome from the review should be the same - which is to ensure the following:

• Bugs/Omissions/Regressions are caught before the change is committed to the source control.
• The change is subjected to keeping the quality of code high so as to make the overall system sustainable. The implementation of the change should be easily
• Bugs/Omissions/Regressions are caught before the change is committed to the source control.
• The change is subjected to keeping the quality of code high so as to make the overall system sustainable. The implementation of the change should be easily readable, documented where necessary, and must favor simplicity of implementation.
• Changes are evaluated from the perspective of a consumer (the reviewer) as opposed to the developer, which often brings out subtleties in the implementation that otherwise go unnoticed.
• The change should be backward compatible and not require extensive work on existing installations in order for it to be consumed. There are exceptions to this in some cases like when work is done on a major release, but otherwise backward compatibility should be upheld at all times. If you are not clear, raise it is as a concern to be clarified during the review.

...

• Your feedback should be clear and actionable. Giving subjective/vague feedback does not add any value or facilitate a constructive dialog.
• Where possible, suggest how your concern can be addressed. For example if your testing revealed that a certain use-case is not satisfied. It is ok to state that as is, but it would be even better if you could suggest how the developer can address it. Present your suggestion as a possible solution rather than the solution.
• If you do not understand part of the change, or for some reason were not able to review part of the change, state it explicitly so as to encourage other reviewers to jump in and help.

Once you have provided your feedback, wait for the developer to respond. It is possible that the developer may need further clarification on your feedback, in which case you should promptly provide it where necessary. In general, the dialog between the reviewer and developer should lead to finding a reasonable middle ground where key concerns are satisfied and the goals of the review have been met.

If a change has met all your criteria for review, please +1 the change to indicate that you are happy with it.

Providing Patches

In order to provide patches, please follow the following guidelines:

• Make sure there is a JIRA: If you are working on fixing a problem that already has an associated JIRA, then go ahead and assign it to yourself. If it is already assigned to someone else, it will be better if you check with the current assignee before moving over to your queue. If the current assignee has already worked out some part of the fix, suggest that you can take that change over from them and complete the remaining parts.

• Attach the patches as you go through development: While small fixes are easily done in a single patch, it is preferable that you attach patches to the JIRA as you go along. This serves as an early feedback mechanism where interested folks can look it over and suggest changes where necessary. It also ensures that if for some reason you are not able to find the time to complete the change, someone else can take up your initial patches and drive them to completion.

• Test your changes before submitting a review: Before you make the JIRA status as 'Patch Available', please test your changes thoroughly. Make sure that all tests are passing and that your the functionality you have worked on is tested through existing or new tests.

• Submitting a patch: To submit a patch, first make sure that you have attached it to the JIRA and changed the status of the JIRA to 'Patch Available'. If the change is non-trivial, then please also post the patch for review on review board. The commands to generate the patch are:

  Code Block
  $ svn diff > /path/to/patch-file.patch

  or

  Code Block
  $ git diff --no-prefix > /path/to/patch-file.patch

• Identify a reviewer: When posting on review board, identify at least one reviewer. You can pick any of the project committers for review. Note that identifying a reviewer does not stop from others from reviewing your change. Be prepared for having your change reviewed by others at any time. If you have posted your change for review and no one has had a chance to review it yet, you can gently remind everyone by dropping a note on the developer mailing list with a link to the review.

• Work with reviewers to get your change fleshed out: When your change is reviewed, please engage with the reviewer via JIRA or review board to get necessary clarifications and work out other details. The goal is to ensure that the final state of your change is acceptable to the reviewer so that they can +1 it.