Skip to end of metadata
Go to start of metadata

This page offers basic guidelines for reviewers who comment on change sets in the Linux Foundation code-review system Gerrit, especially on new bodies of work. Please note the ideal service level agreement (SLA) is that a reviewer shall respond within 36 hours of being added as a reviewer to a change set.

When to post -2, -1, +1, +2

Reviewers indicate their opinion by posting a code-review vote as follows:

  • Major defects: -2, this shall not be merged.  Hopefully you will not ever need to vote this way.
  • Significant concerns: -1, I would prefer this is not merged as is
  • I approve: +1, Looks good to me, but someone else must approve
  • Merge this: +2, Looks good to me, approved

Only committers can vote +2 and merge.  Please note that committers are STRONGLY encouraged to avoid merging their own code if at all possible.

What about 0?  Some people add comments without disapproving or approving. This may be appropriate when the reviewer lacks technical or domain expertise on the feature being changed.

Level 0 Review - Policy Compliance

These criteria check compliance with Linux Foundation and Acumos project policies:

  1. The commit message complies with the required format:
    1. Short first line summary, up to 50 characters
    2. Blank line
    3. Details about the change
    4. Blank line
    5. Issue-Id, Change-Id and Signed-off-by lines are present
  2. No binary files - no exe, jar, tgz, zip or other non-source files.
    1. Exception: picture files like gif, jpg and png are acceptable
  3. The file license.txt is present in the repository root
  4. All text files that support comments have a license statement at the top. For example, this generally includes all source code and RST files, but excludes data files like JSON
  5. No files from third parties are present, especially not files that assert licenses other than Apache 2.0 or compatible

If a change set does not meet this level, I feel a -2 vote is appropriate.

Level 1 Review - Sanity and Reusability

These criteria check if the code in the change set can be used by others:

  1. The file readme.md (markdown format) is present in the repository root and gives an overview of the contents
  2. The file doc/release-notes.rst (Real Simple Text format) is present and contains a concise statement of the change set
  3. The project has build and test specifications; e.g., Maven pom.xml for java and Tox for python
  4. The Jenkins server has a verify job, and the verify job succeeds on the change set
  5. Every server (i.e., non-library) project must have a suitable build specification to yield a Docker image

If a change set does not meet this level, I feel a -1 vote is appropriate.

Level 2 Review - Java Code Practices

These criteria apply to Java code changes. It's impossible to cover all cases here so this section focuses on specific Acumos concerns and some common pitfalls:

  1. Configuration properties should be documented appropriately and an example template should be provided
  2. All code should log debug and errors via the SLF4J library
  3. Servers should include the Logback library and logback configuration file with the recommended output format.
  4. JUnit tests must be included to achieve at least 50% coverage of the statements as measured by Sonar.
  5. Log (don't discard) exceptions.  Stated differently, a catch block must never be empty.
  6. Avoid resource leaks by appropriate use of try-finally constructs.

Level 2 Review - Python Code Practices

These criteria apply to Java code changes

  1. Tox tests must be included to achieve at least 50% coverage of the statements as measured by Sonar
  2. TBD by a true Python expert.

Level 2 Review - CSS, HTML, Javascript and Typescript Code Practices

These criteria apply to web-site code changes

  1. TBD by a true front-end expert.
  • No labels

5 Comments

  1. I recommend we try to automate the Level 0 Review - Policy Compliance.

    The guide should also provide recommendations for:

    • commit message
    • Issue-ID references
    • topic naming (never commit patches to master, use branch ala 'git checkout -b mybranch' and then work on your patch)
    • how to indicate when the patch is ready for merge (or not)
    • use of -1, +1, +2
    • how to help ensure quicker turnaround for patch reviews
    • functional verification by another developer pre-merge
    • general avoidance of +2/merge by patch author
      • except for cases in which this is acceptable
    1. Thanks I have extended the page to incorporate your comments.

  2. Rearding common mistakes that most Java developers make following tips might be useful

    • Double is often preferred over float in software where precision is important because of the following reasons:
      Most processors take nearly the same amount of processing time to perform operations on Float and Double. Double offers far more precision in the same amount of computation time.
    • Do not write very long methods in Java . Perform as much refactoring as possible . Small methods are also easy to test.
    • Avoid memory leaks by using final blocks whenever possible
    • Avoid String Concatenation in a Large Loop
    • Avoid Empty Catch Blocks
    • Test-Driven Development as much as possible
    1. Resource leaks you mean (not memory leaks)?