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
