Hi Matt, thats already in there. See last bullet point of 2.6 20.12.2016, 14:14, "Matt Foley" <[email protected]>: > If we aren't going to require 100% test coverage for new code, then we should > at least say "thorough" automated tests, in the last bullet of 2.6. And it > should be a mandate not an assumption: > > 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 ) > > IMO, > --Matt > > On 12/20/16, 12:51 PM, "James Sirota" <[email protected]> wrote: > > Good feedback. Here is the next iteration that accounts for your > suggestions: > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=61332235 > > 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 [email protected] 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. > 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 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. > 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’re 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? > > 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’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: > 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 automated > tests exist 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 > > 20.12.2016, 13:18, "Otto Fowler" <[email protected]>: > > "The purpose of these policies is to minimize the chances we merge a > change > > that jeopardizes has unintended consequences." > > > > remove jeopardizes? > > > > On December 20, 2016 at 13:25:35, James Sirota ([email protected]) > wrote: > > > > I incorporated the changes. This is what the doc looks like now: > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=61332235 > > > > 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 [email protected] and someone will give you > > permission > > 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 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. > > 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’re 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? > > > > 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’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: > > 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 automated > > tests exist 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 jeopardizes 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 > > > > 20.12.2016, 09:42, "[email protected]" <[email protected]>: > >> I don't have enough experience on the Java/Javadoc side to make a > > > > specific > >> suggestion, but with other languages I've used Sphinx and Doxygen with > >> great results. > >> > >> Jon > >> > >> On Tue, Dec 20, 2016 at 11:29 AM Michael Miklavcic < > >> [email protected]> wrote: > >> > >>> Were you thinking javadoc or something more? I wouldn't mind seeing us > >>> produce a javadoc site, if we aren't already doing so. > >>> > >>> On Dec 20, 2016 9:25 AM, "[email protected]" <[email protected]> wrote: > >>> > >>> > Regarding documentation - while I'm not a huge fan of that approach > > > > (I > >>> > would prefer to see documentation generated from the code), I think > > > > it > >>> > could work in the short term. Having that outlined both in the > coding > >>> > guidelines and on the wiki would be important. > >>> > > >>> > I agree with the comments about author != committer, and 100% code > >>> > coverage. > >>> > > >>> > Jon > >>> > > >>> > On Tue, Dec 20, 2016 at 11:10 AM James Sirota <[email protected]> > >>> wrote: > >>> > > >>> > > In my view the lower-level documentation that should be source > >>> controlled > >>> > > with the code belongs on github and then use case documentation > and > >>> > > top-level architecture diagrams belong on the wiki. What do you > > > > think? > >>> > > > >>> > > I think if the author is not a committer and can't merge then the > >>> > reviewer > >>> > > should probably merge or the PR originator should ping the dev > > > > board to > >>> > get > >>> > > someone to merge the PR in. Does that seem reasonable to everyone? > >>> > > > >>> > > 18.12.2016, 13:10, "Kyle Richardson" <[email protected]>: > >>> > > > Couple of questions/comments: > >>> > > > > >>> > > > In 2.4, we talk about Javadoc and code comments but not too much > >>> about > >>> > > the > >>> > > > user documentation. Should we, possibly in a section 4, give > some > >>> > > > recommendations on what should go into the README files versus > on > > > > the > >>> > > wiki? > >>> > > > This could also help the reviewer know if the change is > > > > documented > >>> > > > sufficiently. > >>> > > > > >>> > > > In 2.6, we say that 1 qualified reviewer (Apache committer or > > > > PPMC > >>> > > member) > >>> > > > other than the author of the PR must have given it a +1. In the > > > > case > >>> > > where > >>> > > > the author is not a committer (who could merge their own PR), > > > > should > >>> we > >>> > > > state that the reviewer will be responsible for the merge? > >>> > > > > >>> > > > -Kyle > >>> > > > > >>> > > > On Fri, Dec 16, 2016 at 6:39 PM, James Sirota > <[email protected]> > > > >>> > > wrote: > >>> > > > > >>> > > >> Lets move this back to the discuss thread since it's still > >>> generating > >>> > > that > >>> > > >> many comments. Please post all your feedback and I will > > > > incorporate > >>> > it > >>> > > and > >>> > > >> put it back to a vote. > >>> > > >> > >>> > > >> Thanks, > >>> > > >> James > >>> > > >> > >>> > > >> 16.12.2016, 16:12, "Matt Foley" <[email protected]>: > >>> > > >> > +1 > >>> > > >> > > >>> > > >> > In 2.2 (follow Sun guidelines), do you want to add the > > > > notation > >>> > > “except > >>> > > >> that indents are 2 spaces instead of 4”, as Hadoop does? Or > does > >>> the > >>> > > Metron > >>> > > >> community like 4-space indents? I see both in the Metron code. > >>> > > >> > > >>> > > >> > My +1 holds in either case. > >>> > > >> > --Matt > >>> > > >> > > >>> > > >> > On 12/16/16, 9:34 AM, "James Sirota" <[email protected]> > > > > wrote: > >>> > > >> > > >>> > > >> > I incorporated the changes to the coding guidelines from our > >>> > discuss > >>> > > >> thread. I'd like to get them voted on to make them official. > >>> > > >> > > >>> > > >> > https://cwiki.apache.org/confluence/pages/viewpage. > >>> > > >> action?pageId=61332235 > >>> > > >> > > >>> > > >> > Please vote +1, -1, 0 > >>> > > >> > > >>> > > >> > The vote will be open for 72 hours. > >>> > > >> > > >>> > > >> > ------------------- > >>> > > >> > Thank you, > >>> > > >> > > >>> > > >> > James Sirota > >>> > > >> > PPMC- Apache Metron (Incubating) > >>> > > >> > jsirota AT apache DOT org > >>> > > >> > >>> > > >> ------------------- > >>> > > >> Thank you, > >>> > > >> > >>> > > >> James Sirota > >>> > > >> PPMC- Apache Metron (Incubating) > >>> > > >> jsirota AT apache DOT org > >>> > > > >>> > > ------------------- > >>> > > Thank you, > >>> > > > >>> > > James Sirota > >>> > > PPMC- Apache Metron (Incubating) > >>> > > jsirota AT apache DOT org > >>> > > > >>> > -- > >>> > > >>> > Jon > >>> > > >>> > Sent from my mobile device > >>> > > >> -- > >> > >> Jon > >> > >> Sent from my mobile device > > > > ------------------- > > Thank you, > > > > James Sirota > > PPMC- Apache Metron (Incubating) > > jsirota AT apache DOT org > > ------------------- > Thank you, > > James Sirota > PPMC- Apache Metron (Incubating) > jsirota AT apache DOT org
------------------- Thank you, James Sirota PPMC- Apache Metron (Incubating) jsirota AT apache DOT org
