[VOTE] Development Guidelines

[James Sirota <js...@apache.org>]

The document is here:

https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=61332235

And is also attached to this email for your convenience.  


Please vote +1, -1, or 0 for neutral.  The vote will be open for 72 hours



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 associated with your pull request, 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 don't have access to JIRA or can't assign an issue to yourself, please message dev@metron.incubator.apache.org and someone will either give you permission or assign a JIRA to you
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.
Also, please indicate impacts to the following (if exist):
System Configuration Changes
Metron Configuration
Metron Component Configuration (sensors, etc)
Tech Stack Configuration (Storm, Hbase, etc)
Environmental Changes
Helper Shell Scripts
RPM Packaging
Ansible, Ambari, AWS, Docker
Documentation Impacts
Changes to Wiki Documentation
Revisions in Tutorials
Developer Guide
Expansions in readme's 
Changes to System Interfaces
Stellar Shell
REST APIs
Etc...
Craft a pull request following the guidelines in Section 2 of this document.  Instructions on how to submit the pull request can be found here. 
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 the person doing the merge (committer) should manually close the corresponding JIRA.
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.  The instructions on how to checkout a patch for review can be found here. 
When reviewing a patch try to keep each of these concepts in mind:
 
Is the proposed change being made in the correct place? Is it a fix in a backend when it should be in the primitives?  In Kafka vs Storm?
What is the change being proposed?  Is it based on Community recognized issues?
Do we want this feature or is the bug they\u2019re fixing really a bug?
Does the change do what the author claims?
Are there sufficient tests?
Has it been documented?
Will this change introduce new bugs?
 
Also, please review if the submitter correctly flagged impacts to the following (if exist):
System Configuration Changes
Metron Configuration
Metron Component Configuration (sensors, etc)
Tech Stack Configuration (Storm, Hbase, etc)
Environmental Changes
Helper Shell Scripts
RPM Packaging
Ansible, Ambari, AWS, Docker
Documentation Impacts
Changes to Wiki Documentation
Revisions in Tutorials
Developer Guide
Expansions in readme's 
Changes to System Interfaces
Stellar Shell
REST APIs
Etc...

2.  Implementation

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\u2019t combine code changes with lots of edits of whitespace or comments; it makes code review too difficult. It\u2019s 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: http://www.oracle.com/technetwork/java/codeconvtoc-136057.html
except that indents are 2 spaces instead of 4
2.3  Coding Standards
Implementation matches what the documentation says  
Logger name is effectively the result of Class.getName() 
Class & member access - as restricted as it can be (subject to testing requirements)  
Appropriate NullPointerException and IllegalArgumentException argument checks  
Asserts - verify they should always be true  
Look for accidental propagation of exceptions  
Look for unanticipated runtime exceptions  
Try-finally used as necessary to restore consistent state  
Logging levels conform to Log4j levels 
Possible deadlocks - look for inconsistent locking order  
Race conditions - look for missing or inadequate synchronization  
Consistent synchronization - always locking the same object(s)  
Look for synchronization or documentation saying there's no synchronization  
Look for possible performance problems  
Look at boundary conditions for problems  
Configuration entries are retrieved/set via setter/getter methods  
Implementation details do NOT leak into interfaces  
Variables and arguments should be interfaces where possible  
If equals is overridden then hashCode is overridden (and vice versa)  
Objects are checked (instanceof) for appropriate type before casting (use generics if possible)  
Public API changes have been publicly discussed  
Use of static member variables should be used with caution especially in Map/reduce tasks due to the JVM reuse feature 
2.4 Documentation

Code-Level Documentation
Self-documenting code (variable, method, class) has a clear semantic name
Accurate, sufficient for developers to code against
Follows standard Javadoc conventions
Loggers and logging levels covered if they do not follow our conventions (see below)
System properties, configuration options, and resources covered
Illegal arguments are properly documented as appropriate
Package and overview Javadoc are updated as appropriate
Javadoc comments are mandatory for all public APIs
Generate Javadocs for release builds

Feature-level documentation -  should be version controlled in github in README files.
Accurate description of the feature
Sample configuration and deployment options
Sample usage scenarios 

High-Level Design documentation - architecture description and diagrams should be a part of a wiki entry.
Provide diagrams/charts where appropriate.  Visuals are always welcome
Provide purpose of the feature/module and why it exists within the project
Describe system flows through the feature/module where appropriate
Describe how the feature/module interfaces with the rest of the system
Describe appropriate usages scenarios and use cases

Tutorials - system-level tutorials and use cases should also be kept as wiki entries.
Add to the Metron reference application documentation for each additional major feature
If appropriate, publish a tutorials blog on the Wiki to highlight usage scenarios and apply them to the real world use cases
2.5  Tests
Unit tests exist for bug fixes and new features, or a rationale is given in JIRA for why there is no test 
 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 merge their own pull request, but only after a second reviewer has given it a +1. A qualified reviewer is a Metron committer or PPMC member.
A non-committer may ask the reviewer to merge their pull request or alternatively post to the Metron dev board to get another committer to merge the PR if the reviewer is not available. 
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 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 will be reviewed with the expectation that thorough automated tests shall be provided and are consistent with project testing methodology and practices, and cover the appropriate cases ( see reviewers guide )

The purpose of these policies is to minimize the chances we merge a change that has unintended consequences.
 
3.  JIRA
The Incompatible change flag on the issue's 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 

-------------------�
Thank you,

James Sirota
PPMC- Apache Metron (Incubating)
jsirota AT apache DOT org