Versions Compared

Key

  • This line was added.
  • This line was removed.
  • Formatting was changed.
Comment: Minor grammar changes

As an open source project, Metron welcomes contributions of all forms. The sections below will help you get started.

1. How To Contribute

We are always very happy to have contributions, whether for trivial cleanups, little additions or big new features.

If you don't know Java or Scala you can still contribute to the project. We strongly value documentation and gladly accept improvements to the documentation.

1.1  Contributing A Code Change

To submit a change for inclusion, please do the following:

  • If there is not already a JIRA a JIRA associated with your pull request, created create it, assign it to yourself, and start progress
  • If there is a JIRA already created for your change then assign it to yourself and start progress
  • If you are introducing a completely new feature or API it is a good idea to start a discussion and get consensus on the basic design first.  Larger changes should be discussed on the dev boards before submission.
  • New features and significant bug fixes should be documented in the JIRA and appropriate architecture diagrams should be attached.  Major features may require a vote.
  • Note that if the change is related to user-facing protocols / interface / configs, etc, you need to make the corresponding change on the documentation as well.
  • Craft a pull request following the guidelines in Section 2 of this document
  • Pull requests should be small to facilitate easier review. Studies have shown that review quality falls off as patch size grows. Sometimes this will result in many small PRs to land a single large feature.
  • People will review and comment on your pull request.  It is our job to follow up on pull requests in a timely fashion.
  • Once the pull request is merged, manually close the corresponding JiraJIRA

1.2 Reviewing and merging patches

Everyone is encouraged to review open pull requests. We only ask that you try and think carefully, ask questions and are excellent to one another. Code review is our opportunity to share knowledge, design ideas and make friends.

When reviewing a patch try to keep each of these concepts in mind:

...

2.1  Grammar and style

These are small things that are not caught by the automated style checkers.

  • Does a variable need a better name?
  • Should this be a keyword argument?
  • In a PR, maintain the existing style of the file.
  • Don’t combine code changes with lots of edits of whitespace or comments; it makes code review too difficult. It’s okay to fix an occasional comment or indenting, but if wholesale comment or whitespace changes are needed, make them a separate PR.
  • Use the checkstyle plugin in Maven to verify that your PR conforms to our style

2.2  Code Style

Follow the Sun Code Conventions outlined here.  

2.3  Coding

  • implementation Implementation matches what the documentation says  
  • logger Logger name is effectively the result of Class.getName() 

  • class Class & member access - as restricted as it can be (subject to testing requirements)  

  • appropriate NullPointerException and IllegalArgumentException argument Appropriate NullPointerException and IllegalArgumentException argument checks  

  • asserts Asserts - verify they should always be true  
  • look Look for accidental propagation of exceptions  
  • look Look for unanticipated runtime exceptions  
  • tryTry-finally used as necessary to restore consistent state  
  • logging Logging levels conform to Log4j levels 

  • possible Possible deadlocks - look for inconsistent locking order  
  • race Race conditions - look for missing or inadequate synchronization  
  • consistent Consistent synchronization - always locking the same object(s)  
  • look Look for synchronization or documentation saying there's no synchronization  
  • look Look for possible performance problems  
  • look Look at boundary conditions for problems  
  • configuration Configuration entries are retrieved/set via setter/getter methods  
  • implementation Implementation details do NOT leak into interfaces  
  • variables Variables and arguments should be interfaces where possible  
  • if equals is If equals is overridden then hashCode is overridden (and vice versa)  

  • objects Objects are checked (instanceof) for appropriate type before casting (use generics if possible)  

  • public Public API changes have been publically publicly discussed  
  • use Use of static member variables should be used with caution especially in Map/reduce tasks due to the JVM reuse feature 

2.4 Documentation

  • accurateAccurate, sufficient for developers to code against
  • follows Follows standard javadoc Javadoc conventions
  • loggers Loggers and logging levels covered if they do not follow our conventions (see below)
  • system System properties, configuration options, and resources covered
  • illegal Illegal arguments are properly documented as appropriate
  • package Package and overview javadoc Javadoc are updated as appropriate
  • Javadoc comments are mandatory for all public APIs
  • Generate Javadocs for release builds

2.5  Tests

  • unit Unit tests exist for bug fixes and new features, or a rationale is given in Jira JIRA for why there is no test 

  • unit Unit tests do not write any temporary files to /tmp (instead, the tests should write to the location specified by the test.build.data system property)  

     

2.6  Merge requirements

Because Metron is so complex, and the implications of getting it wrong so devastating, Metron has a strict merge policy for committers:

  • Patches must never be pushed directly to master, all changes (even the most trivial typo fixes!) must be submitted as a pull request.
  • A committer may never merge their own pull request, a second party must merge their changes after it has be properly reviewed. 
  • There should be at least one independent party besides the committer that have reviewed the patch before merge.
  • A patch that breaks tests, or introduces regressions by changing or removing existing tests should not be merged. Tests must always be passing on master. This implies that the tests have been run...
  •  All All pull request submitters must link to travis-ci 
  • If somehow the tests get into a failing state on master (such as by a backwards incompatible release of a dependency) no pull requests may be merged until this is rectified.
  • All merged patches must have 100% test coverage.

The purpose of these policies is to minimize the chances we merge a change that jeopardizes has unintended concequencesconsequences.

 

3.  Jira JIRA

  • the Incompatible change flag on the issue's Jira JIRA is set appropriately for this patch  

  • for incompatible changes, major features/improvements, and other release notable issues, the Release Note field has a sufficient comment 

...