18.12. Submitting Patches

HBase moved to GIT from SVN. Until we develop our own documentation for how to contribute patches in our new GIT context, caveat the fact that we have a different branching modes and that we don't currently do the merge practice described in the following, the accumulo doc on how to contribute and develop after our move to GIT is worth a read.

If you are new to submitting patches to open source or new to submitting patches to Apache, I'd suggest you start by reading the On Contributing Patches page from Apache Commons Project. Its a nice overview that applies equally to the Apache HBase Project.

18.12.1. Create Patch

See the aforementioned Apache Commons link for how to make patches against a checked out subversion repository. Patch files can also be easily generated from Eclipse, for example by selecting "Team -> Create Patch". Patches can also be created by git diff and svn diff.

Make patches against the master branch. Do the master branch first even if you want the patch in another branch altogether. We operate by apply fixes first to the master branch and then backporting. Another reason to patch master first is because our patch testing system only works against the master branch. Attach your patch to your JIRA and then mark the issue 'Patch Available'. This will prompt the HadoopQA system to run your patch through all unit tests and report back the results to the JIRA. It may take a few hours for your patch to be run. HadoopQA runs the most recently attached patch to the JIRA regardless. If you want to rerun your patch against HadoopQA perhaps to verify that indeed a certain test failure is the result of your patch, then reattach your patch and it will be run again.

Please submit one patch-file per Jira. For example, if multiple files are changed make sure the selected resource when generating the patch is a directory. Patch files can reflect changes in multiple files.

Generating patches using git:

$ git diff --no-prefix  > HBASE_XXXX.patch

Don't forget the 'no-prefix' option; and generate the diff from the root directory of project

Make sure you review Section 18.3.1.1, “Code Formatting” for code style.

18.12.2. Patch File Naming

The patch file should have the Apache HBase Jira ticket in the name. For example, if a patch was submitted for Foo.java, then a patch file called Foo_HBASE_XXXX.patch would be acceptable where XXXX is the Apache HBase Jira number.

If you generating from a branch, then including the target branch in the filename is advised, e.g., HBASE_XXXX-0.90.patch.

18.12.3. Unit Tests

Yes, please. Please try to include unit tests with every code patch (and especially new classes and large changes). Make sure unit tests pass locally before submitting the patch.

Also, see Section 18.11.2.2, “Mockito”.

If you are creating a new unit test class, notice how other unit test classes have classification/sizing annotations at the top and a static method on the end. Be sure to include these in any new unit test files you generate. See Section 18.8, “Tests” for more on how the annotations work.

18.12.4. Attach Patch to Jira

The patch should be attached to the associated Jira ticket "More Actions -> Attach Files". Make sure you click the ASF license inclusion, otherwise the patch can't be considered for inclusion.

Once attached to the ticket, click "Submit Patch" and the status of the ticket will change. Committers will review submitted patches for inclusion into the codebase. Please understand that not every patch may get committed, and that feedback will likely be provided on the patch. Fear not, though, because the Apache HBase community is helpful!

18.12.5. Common Patch Feedback

The following items are representative of common patch feedback. Your patch process will go faster if these are taken into account before submission.

See the Java coding standards for more information on coding conventions in Java.

18.12.5.1. Space Invaders

Rather than do this...

if ( foo.equals( bar ) ) {     // don't do this

... do this instead...

if (foo.equals(bar)) {

Also, rather than do this...

foo = barArray[ i ];     // don't do this

... do this instead...

foo = barArray[i];

18.12.5.2. Auto Generated Code

Auto-generated code in Eclipse often looks like this...

 public void readFields(DataInput arg0) throws IOException {    // don't do this
   foo = arg0.readUTF();                                       // don't do this

... do this instead ...

 public void readFields(DataInput di) throws IOException {
   foo = di.readUTF();

See the difference? 'arg0' is what Eclipse uses for arguments by default.

18.12.5.3. Long Lines

Keep lines less than 100 characters.

Bar bar = foo.veryLongMethodWithManyArguments(argument1, argument2, argument3, argument4, argument5, argument6, argument7, argument8, argument9);  // don't do this

... do something like this instead ...

Bar bar = foo.veryLongMethodWithManyArguments(
 argument1, argument2, argument3,argument4, argument5, argument6, argument7, argument8, argument9);

18.12.5.4. Trailing Spaces

This happens more than people would imagine.

Bar bar = foo.getBar();     <--- imagine there is an extra space(s) after the semicolon instead of a line break.

Make sure there's a line-break after the end of your code, and also avoid lines that have nothing but whitespace.

18.12.5.5. Implementing Writable

Applies pre-0.96 only

In 0.96, HBase moved to protobufs. The below section on Writables applies to 0.94.x and previous, not to 0.96 and beyond.

Every class returned by RegionServers must implement Writable. If you are creating a new class that needs to implement this interface, don't forget the default constructor.

18.12.5.6. Javadoc

This is also a very common feedback item. Don't forget Javadoc!

Javadoc warnings are checked during precommit. If the precommit tool gives you a '-1', please fix the javadoc issue. Your patch won't be committed if it adds such warnings.

18.12.5.7. Findbugs

Findbugs is used to detect common bugs pattern. As Javadoc, it is checked during the precommit build up on Apache's Jenkins, and as with Javadoc, please fix them. You can run findbugs locally with 'mvn findbugs:findbugs': it will generate the findbugs files locally. Sometimes, you may have to write code smarter than Findbugs. You can annotate your code to tell Findbugs you know what you're doing, by annotating your class with:

@edu.umd.cs.findbugs.annotations.SuppressWarnings(
                    value="HE_EQUALS_USE_HASHCODE",
                    justification="I know what I'm doing")

Note that we're using the apache licensed version of the annotations.

18.12.5.8. Javadoc - Useless Defaults

Don't just leave the @param arguments the way your IDE generated them. Don't do this...

  /**
   *
   * @param bar             <---- don't do this!!!!
   * @return                <---- or this!!!!
   */
  public Foo getFoo(Bar bar);

... either add something descriptive to the @param and @return lines, or just remove them. But the preference is to add something descriptive and useful.

18.12.5.9. One Thing At A Time, Folks

If you submit a patch for one thing, don't do auto-reformatting or unrelated reformatting of code on a completely different area of code.

Likewise, don't add unrelated cleanup or refactorings outside the scope of your Jira.

18.12.5.10. Ambigious Unit Tests

Make sure that you're clear about what you are testing in your unit tests and why.

18.12.6. Submitting a patch again

Sometimes committers ask for changes for a patch. After incorporating the suggested/requested changes, follow the following process to submit the patch again.

  • Do not delete the old patch file

  • version your new patch file using a simple scheme like this:

    HBASE-{jira number}-{version}.patch

    e.g:

    HBASE_XXXX-v2.patch
  • 'Cancel Patch' on JIRA.. bug status will change back to Open

  • Attach new patch file (e.g. HBASE_XXXX-v2.patch) using 'Files --> Attach'

  • Click on 'Submit Patch'. Now the bug status will say 'Patch Available'.

Committers will review the patch. Rinse and repeat as many times as needed :-)

18.12.7. Submitting incremental patches

At times you may want to break a big change into mulitple patches. Here is a sample work-flow using git

  • patch 1:

    • $ git diff --no-prefix > HBASE_XXXX-1.patch
  • patch 2:

    • create a new git branch

      $ git checkout -b my_branch
    • save your work

      $ git add file1 file2 
      $ git commit -am 'saved after HBASE_XXXX-1.patch'

      now you have your own branch, that is different from remote master branch

    • make more changes...

    • create second patch

      $ git diff --no-prefix > HBASE_XXXX-2.patch

18.12.8. ReviewBoard

Larger patches should go through ReviewBoard.

For more information on how to use ReviewBoard, see the ReviewBoard documentation.

18.12.9. Guide for HBase Committers

18.12.9.1. New committers

New committers are encouraged to first read Apache's generic committer documentation:

18.12.9.2. Review

HBase committers should, as often as possible, attempt to review patches submitted by others. Ideally every submitted patch will get reviewed by a committer within a few days. If a committer reviews a patch they've not authored, and believe it to be of sufficient quality, then they can commit the patch, otherwise the patch should be cancelled with a clear explanation for why it was rejected.

The list of submitted patches is in the HBase Review Queue. This is ordered by time of last modification. Committers should scan the list from top-to-bottom, looking for patches that they feel qualified to review and possibly commit.

For non-trivial changes, it is required to get another committer to review your own patches before commit. Use "Submit Patch" like other contributors, and then wait for a "+1" from another committer before committing.

18.12.9.3. Reject

Patches should be rejected which do not adhere to the guidelines in HowToContribute and to the code review checklist. Committers should always be polite to contributors and try to instruct and encourage them to contribute better patches. If a committer wishes to improve an unacceptable patch, then it should first be rejected, and a new patch should be attached by the committer for review.

18.12.9.4. Commit

Committers commit patches to the Apache HBase GIT repository.

Before you commit!!!!

Make sure your local configuration is correct. In particular, your identity and email. Do $ git config --list. Check what shows as your user.email and user.name. See this GitHub article, Set Up Git if you need pointers.

When you commit a patch, please:

  1. Include the Jira issue id in the commit message, along with a short description of the change and the name of the contributor if it is not you. Be sure to get the issue id right, as this causes Jira to link to the change in Subversion (use the issue's "All" tab to see these).

  2. Resolve the issue as fixed, thanking the contributor. Always set the "Fix Version" at this point, but please only set a single fix version, the earliest release in which the change will appear.

18.12.9.4.1. Add Amending-Author when a conflict cherrypick backporting

We've established the practice of committing to trunk and then cherry picking back to branches whenever possible. When there is a minor conflict we can fix it up and just proceed with the commit. The resulting commit retains the original author. When the amending author is different from the original committer, add notice of this at the end of the commit message as: Amending-Author: Author <committer&apache> See discussion at HBase, mail # dev - [DISCUSSION] Best practice when amending commits cherry picked from master to branch.

18.12.9.4.2. Committers are responsible for making sure commits do not break the build or tests

If a committer commits a patch it is their responsibility to make sure it passes the test suite. It is helpful if contributors keep an eye out that their patch does not break the hbase build and/or tests but ultimately, a contributor cannot be expected to be up on the particular vagaries and interconnections that occur in a project like hbase. A committer should.

18.12.9.4.3. Patching Etiquette

In the thread HBase, mail # dev - ANNOUNCEMENT: Git Migration In Progress (WAS => Re: Git Migration), it was agreed on the following patch flow

  1. Develop and commit the patch against trunk/master first.

  2. Try to cherry-pick the patch when backporting if possible.

  3. If this does not work, manually commit the patch to the branch.

18.12.9.4.4. Committing Documentation

TBS

18.12.10. Dialog

Committers should hang out in the #hbase room on irc.freenode.net for real-time discussions. However any substantive discussion (as with any off-list project-related discussion) should be re-iterated in Jira or on the developer list.

18.12.11. Do not edit JIRA comments

Misspellings and/or bad grammar is preferable to the disruption a JIRA comment edit causes: See the discussion at Re:(HBASE-451) Remove HTableDescriptor from HRegionInfo

comments powered by Disqus