metron-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From James Sirota <jsir...@apache.org>
Subject Re: [DISCUSS] Coding Guidelines
Date Wed, 04 Jan 2017 21:54:13 GMT
Does anyone else has any comments or objections to the document?  Or can we put this up for a vote?

21.12.2016, 13:24, "Kyle Richardson" <kylerichardson2@gmail.com>:
> +1 Thanks for the updates to the documentation and testing sections.
>
> -Kyle
>
> On Wed, Dec 21, 2016 at 3:17 PM, Otto Fowler <ottobackwards@gmail.com>
> wrote:
>
>>  +1
>>
>>  On December 21, 2016 at 14:56:06, Michael Miklavcic (
>>  michael.miklavcic@gmail.com) wrote:
>>
>>  Works for me also.
>>
>>  On Wed, Dec 21, 2016 at 12:38 PM, Matt Foley <mattf@apache.org> wrote:
>>
>>  > Works for me, thanks.
>>  >
>>  > On 12/21/16, 11:21 AM, "Casey Stella" <cestella@gmail.com> wrote:
>>  >
>>  > Sure, how about making it generic to "a deployed cluster"?
>>  >
>>  > On Wed, Dec 21, 2016 at 2:20 PM, Matt Foley <mattf@apache.org> wrote:
>>  >
>>  > > +1 on Casey’s first edit. However, wrt the second, can we please not
>>  > > require vagrant? Any of our single-node test deployments, including
>>  > > vagrant, ansible, mpack, or (soon :-) docker, should be acceptable.
>>  > >
>>  > > Thanks,
>>  > > --Matt (who can’t run vagrant workably on the systems available to
>>  > me)
>>  > >
>>  > >
>>  > > On 12/21/16, 8:52 AM, "Michael Miklavcic" <
>>  > michael.miklavcic@gmail.com>
>>  > > wrote:
>>  > >
>>  > > Agreed on Casey's addition to 2.5. What do you think about
>>  > saying the
>>  > > pla
>>  > > should be stated on the PR, since that will be replicated to Jira
>>  > > automatically?
>>  > >
>>  > > On Wed, Dec 21, 2016 at 7:49 AM, Casey Stella <
>>  > cestella@gmail.com>
>>  > > wrote:
>>  > >
>>  > > > Oh, one more, I propose the following addition to 2.5:
>>  > > > >
>>  > > > > JIRAs will have a description of how to exercise the
>>  > functionality
>>  > > in a
>>  > > > > step-by-step manner on a Quickdev vagrant instance to aid
>>  > review
>>  > > and
>>  > > > > validation.
>>  > > >
>>  > > >
>>  > > > When Mike, Otto and I moved the system to the current version
>>  > of
>>  > > Storm, we
>>  > > > needed a broader smoke test than just running data through that
>>  > > exercised a
>>  > > > variety of the features. We pulled those smoke tests from the
>>  > various
>>  > > > discussions in the JIRAs.
>>  > > >
>>  > > >
>>  > > >
>>  > > > On Wed, Dec 21, 2016 at 9:38 AM, Casey Stella <
>>  > cestella@gmail.com>
>>  > > wrote:
>>  > > >
>>  > > > > We have been having a lively discussion on METRON-590 (see
>>  > > > > https://github.com/apache/incubator-metron/pull/395) around
>>  > > creating
>>  > > > > multiple abstractions to do the same (or very nearly the
>>  > same)
>>  > > thing.
>>  > > > >
>>  > > > > I'd like to propose an addition to section 2.3 which reads:
>>  > > > >
>>  > > > >> Contributions which provide abstractions which are either
>>  > very
>>  > > similar
>>  > > > to
>>  > > > >> or a subset of existing abstractions should use and extend
>>  > > existing
>>  > > > >> abstractions rather than provide competing abstractions
>>  > unless
>>  > > > engineering
>>  > > > >> exigencies (e.g. performance ) make such an operation
>>  > impossible
>>  > > without
>>  > > > >> compromising core functionality of the platform.
>>  > > > >
>>  > > > >
>>  > > > > I'd like to suggest the following anecdote from the early
>>  > years of
>>  > > the
>>  > > > > codebase to justify the above:
>>  > > > >
>>  > > > > Stellar started as a predicate language only for threat
>>  > triage
>>  > > rules. As
>>  > > > > such, when the task of creating Field Transformations came
>>  > to me, I
>>  > > > needed
>>  > > > > something like Stellar except I needed it to return arbitrary
>>  > > objects,
>>  > > > > rather than just booleans. In my infinite wisdom, I chose to
>>  > fork
>>  > > the
>>  > > > > language, create a second, more specific DSL for field
>>  > > transformations,
>>  > > > > thereby creating "Metron Query Language" and "Metron
>>  > Transformation
>>  > > > > Language."
>>  > > > >
>>  > > > > I felt a nagging feeling at the time that I should just
>>  > expand the
>>  > > query
>>  > > > > language, but I convinced myself that it would require too
>>  > much
>>  > > testing
>>  > > > and
>>  > > > > it would be a change that was too broad in scope. It took 3
>>  > months
>>  > > for me
>>  > > > > to get around to unifying those languages and if we had more
>>  > > people using
>>  > > > > it, it would have been an absolute nightmare.
>>  > > > >
>>  > > > > On Wed, Dec 21, 2016 at 9:31 AM, Casey Stella <
>>  > cestella@gmail.com>
>>  > > > wrote:
>>  > > > >
>>  > > > >> Yeah, I +1 the notion of thorough automated tests.
>>  > > > >>
>>  > > > >> On Tue, Dec 20, 2016 at 4:36 PM, Matt Foley <
>>  > mattf@apache.org>
>>  > > 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" <jsirota@apache.org>
>>  > wrote:
>>  > > > >>>
>>  > > > >>> Hi Matt, thats already in there. See last bullet point
>>  > of 2.6
>>  > > > >>>
>>  > > > >>> 20.12.2016, 14:14, "Matt Foley" <mattf@apache.org>:
>>  > > > >>> > 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" <
>>  > jsirota@apache.org>
>>  > > > wrote:
>>  > > > >>> >
>>  > > > >>> > Good feedback. Here is the next iteration that
>>  > > accounts for
>>  > > > >>> your suggestions:
>>  > > > >>> > https://cwiki.apache.org/
>>  > confluence/pages/viewpage.
>>  > > action?p
>>  > > > >>> ageId=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 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.
>>  > > > >>> > 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" <
>>  > > ottobackwards@gmail.com>:
>>  > > > >>> > > "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
>>  > (
>>  > > > >>> jsirota@apache.org) wrote:
>>  > > > >>> > >
>>  > > > >>> > > I incorporated the changes. This is what the
>>  > doc
>>  > > looks like
>>  > > > >>> now:
>>  > > > >>> > > https://cwiki.apache.org/
>>  > confluence/pages/viewpage.
>>  > > > action?pa
>>  > > > >>> geId=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 dev@metron.incubator.apache.org
>>  > 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.ht
>>  > > > >>> ml
>>  > > > >>> > > 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, "Zeolla@GMail.com" <
>>  > > zeolla@gmail.com>:
>>  > > > >>> > >> 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 <
>>  > > > >>> > >> michael.miklavcic@gmail.com> 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, "Zeolla@GMail.com"
>>  > <
>>  > > > >>> zeolla@gmail.com> 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 <
>>  > > > >>> jsirota@apache.org>
>>  > > > >>> > >>> 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" <
>>  > > > >>> kylerichardson2@gmail.com>:
>>  > > > >>> > >>> > > > 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 <
>>  > > > >>> jsirota@apache.org>
>>  > > > >>> > >
>>  > > > >>> > >>> > > 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" <
>>  > > mattf@apache.org
>>  > > > >:
>>  > > > >>> > >>> > > >> > +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" <
>>  > > > >>> jsirota@apache.org>
>>  > > > >>> > >
>>  > > > >>> > > 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/confl
>>  > > > >>> uence/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
>>  > > > >>>
>>  > > > >>>
>>  > > > >>>
>>  > > > >>>
>>  > > > >>>
>>  > > > >>
>>  > > > >
>>  > > >
>>  > >
>>  > >
>>  > >
>>  > >
>>  > >
>>  >
>>  >
>>  >
>>  >
>>  >

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

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


Mime
View raw message