Yeah, I +1 the notion of thorough automated tests. On Tue, Dec 20, 2016 at 4:36 PM, Matt Foley <[email protected]> wrote:
> Hard to mark diffs in text-only mode :-) My proposed change is: > > >> All merged patches will be reviewed with the expectation that thorough > automated tests shall be provided and are consistent with … > > ^^^^^^^^ > ^^^^^^^^^^^^^^ > Added word “thorough” and changed “exist” to “shall be provided”. > Thanks, > --Matt > > On 12/20/16, 1:22 PM, "James Sirota" <[email protected]> wrote: > > 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 > > > > >
