Re: [DISCUSS] Coding Guidelines

[James Sirota]

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" :
> +1 Thanks for the updates to the documentation and testing sections.
>
> -Kyle
>
> On Wed, Dec 21, 2016 at 3:17 PM, Otto Fowler 
> wrote:
>
>>  +1
>>
>>  On December 21, 2016 at 14:56:06, Michael Miklavcic (
>>  michael.miklav...@gmail.com) wrote:
>>
>>  Works for me also.
>>
>>  On Wed, Dec 21, 2016 at 12:38 PM, Matt Foley  wrote:
>>
>>  > Works for me, thanks.
>>  >
>>  > On 12/21/16, 11:21 AM, "Casey Stella"  wrote:
>>  >
>>  > Sure, how about making it generic to "a deployed cluster"?
>>  >
>>  > On Wed, Dec 21, 2016 at 2:20 PM, Matt Foley  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.miklav...@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 <
>>  > ceste...@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 <
>>  > ceste...@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 <
>>  > ceste...@gmail.com>
>>  > > > wrote:
>>  > > > >
>>  > > > >> Yeah, I +1 the notion of thorough automated tests.
>>  > > > >>
>>  > > > >> On Tue, Dec 20, 2016 at 4:36 PM, Matt Foley <
>>  > ma...@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
>>  > 

Re: [DISCUSS] Coding Guidelines

[Otto Fowler]

+1



On December 21, 2016 at 14:56:06, Michael Miklavcic (
michael.miklav...@gmail.com) wrote:

Works for me also.

On Wed, Dec 21, 2016 at 12:38 PM, Matt Foley  wrote:

> Works for me, thanks.
>
> On 12/21/16, 11:21 AM, "Casey Stella"  wrote:
>
> Sure, how about making it generic to "a deployed cluster"?
>
> On Wed, Dec 21, 2016 at 2:20 PM, Matt Foley  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.miklav...@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 <
> ceste...@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 <
> ceste...@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 <
> ceste...@gmail.com>
> > > wrote:
> > > >
> > > >> Yeah, I +1 the notion of thorough automated tests.
> > > >>
> > > >> On Tue, Dec 20, 2016 at 4:36 PM, Matt Foley <
> ma...@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" 
> wrote:
> > > >>>
> > > >>> Hi Matt, thats already in there. See last bullet point
> of 2.6
> > > >>>
> > > >>> 20.12.2016, 14:14, "Matt Foley" :
> > > >>> > 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 

Re: [DISCUSS] Coding Guidelines

[Michael Miklavcic]

Works for me also.

On Wed, Dec 21, 2016 at 12:38 PM, Matt Foley  wrote:

> Works for me, thanks.
>
> On 12/21/16, 11:21 AM, "Casey Stella"  wrote:
>
> Sure, how about making it generic to "a deployed cluster"?
>
> On Wed, Dec 21, 2016 at 2:20 PM, Matt Foley  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.miklav...@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 <
> ceste...@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 <
> ceste...@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 <
> ceste...@gmail.com>
> > > wrote:
> > > >
> > > >> Yeah, I +1 the notion of thorough automated tests.
> > > >>
> > > >> On Tue, Dec 20, 2016 at 4:36 PM, Matt Foley <
> ma...@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, 

Re: [DISCUSS] Coding Guidelines

[Matt Foley]

Works for me, thanks.

On 12/21/16, 11:21 AM, "Casey Stella"  wrote:

Sure, how about making it generic to "a deployed cluster"?

On Wed, Dec 21, 2016 at 2:20 PM, Matt Foley  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" 
> 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 
> 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 
> 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 
> > wrote:
> > >
> > >> Yeah, I +1 the notion of thorough automated tests.
> > >>
> > >> On Tue, Dec 20, 2016 at 4:36 PM, Matt Foley 
> 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"  wrote:
> > >>>
> > >>> Hi Matt, thats already in there. See last bullet point of 
2.6
> > >>>
> > >>> 20.12.2016, 14:14, "Matt Foley" :
> > >>> > If we aren't going to require 100% test coverage for new
> code,
> > >>> then we should at least say 

Re: [DISCUSS] Coding Guidelines

[Casey Stella]

Sure, how about making it generic to "a deployed cluster"?

On Wed, Dec 21, 2016 at 2:20 PM, Matt Foley  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" 
> 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 
> 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 
> 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 
> > wrote:
> > >
> > >> Yeah, I +1 the notion of thorough automated tests.
> > >>
> > >> On Tue, Dec 20, 2016 at 4:36 PM, Matt Foley 
> 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"  wrote:
> > >>>
> > >>> Hi Matt, thats already in there. See last bullet point of 2.6
> > >>>
> > >>> 20.12.2016, 14:14, "Matt Foley" :
> > >>> > 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" 

Re: [DISCUSS] Coding Guidelines

[Matt Foley]

+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"  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  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  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 
> wrote:
> >
> >> Yeah, I +1 the notion of thorough automated tests.
> >>
> >> On Tue, Dec 20, 2016 at 4:36 PM, Matt Foley  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"  wrote:
> >>>
> >>> Hi Matt, thats already in there. See last bullet point of 2.6
> >>>
> >>> 20.12.2016, 14:14, "Matt Foley" :
> >>> > 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" 
> 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 

Re: [DISCUSS] Coding Guidelines

[Michael Miklavcic]

Agreed on Casey's addition to 2.5. What do you think about saying the plan
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  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  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 
> wrote:
> >
> >> Yeah, I +1 the notion of thorough automated tests.
> >>
> >> On Tue, Dec 20, 2016 at 4:36 PM, Matt Foley  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"  wrote:
> >>>
> >>> Hi Matt, thats already in there. See last bullet point of 2.6
> >>>
> >>> 20.12.2016, 14:14, "Matt Foley" :
> >>> > 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" 
> 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 

Re: [DISCUSS] Coding Guidelines

[Casey Stella]

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  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  wrote:
>
>> Yeah, I +1 the notion of thorough automated tests.
>>
>> On Tue, Dec 20, 2016 at 4:36 PM, Matt Foley  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"  wrote:
>>>
>>> Hi Matt, thats already in there. See last bullet point of 2.6
>>>
>>> 20.12.2016, 14:14, "Matt Foley" :
>>> > 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"  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 

Re: [DISCUSS] Coding Guidelines

[Casey Stella]

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  wrote:

> Yeah, I +1 the notion of thorough automated tests.
>
> On Tue, Dec 20, 2016 at 4:36 PM, Matt Foley  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"  wrote:
>>
>> Hi Matt, thats already in there. See last bullet point of 2.6
>>
>> 20.12.2016, 14:14, "Matt Foley" :
>> > 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"  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 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 

Re: [DISCUSS] Coding Guidelines

[Matt Foley]

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"  wrote:

Hi Matt, thats already in there. See last bullet point of 2.6

20.12.2016, 14:14, "Matt Foley" :
> 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"  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 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: 

Re: [DISCUSS] Coding Guidelines

[James Sirota]

Hi Matt, thats already in there. See last bullet point of 2.6

20.12.2016, 14:14, "Matt Foley" :
> 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"  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 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 

Re: [DISCUSS] Coding Guidelines

[Matt Foley]

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"  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 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 

Re: [DISCUSS] Coding Guidelines

[James Sirota]

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 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 

Re: [DISCUSS] Coding Guidelines

[Otto Fowler]

"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 (jsir...@apache.org) 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 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.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 

Re: [DISCUSS] Coding Guidelines

[Otto Fowler]

Once the pull request is merged, manually close the corresponding JIRA

Again, the submitter may not have rights to assign themselves the jira to 
close, even if they have rights to create.  We need to be clearer on this part 
of the process.

Does the person doing the merge assign the jira to themselves for example?



On December 20, 2016 at 13:25:35, James Sirota (jsir...@apache.org) 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 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.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 

Re: [DISCUSS] Coding Guidelines

[zeo...@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.miklav...@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, "zeo...@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 
> 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" :
> > > > 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 
> > > 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" :
> > > >>  > +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"  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

Re: [DISCUSS] Coding Guidelines

[Michael Miklavcic]

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, "zeo...@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  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" :
> > > 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 
> > 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" :
> > >>  > +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"  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
>

Re: [DISCUSS] Coding Guidelines

[Casey Stella]

+1 to that.

On Tue, Dec 20, 2016 at 11:23 Michael Miklavcic 
wrote:

> Just want to make sure the change to remove "100% code coverage" makes it
>
> in. I prefer something along the lines of the language Otto and I were
>
> discussing:
>
>
>
> "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 )"
>
>
>
> On Dec 20, 2016 9:10 AM, "James Sirota"  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" :
>
> > 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 
> 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" :
>
> >>  > +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"  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
>
>

Re: [DISCUSS] Coding Guidelines

[James Sirota]

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" :
> 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  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" :
>>  > +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"  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

[DISCUSS] Coding Guidelines

[Kyle Richardson]

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  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" :
> > +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"  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
>

Re: [DISCUSS] Coding Guidelines

[Otto Fowler]

Given that we have been seeing problems that appear only in travis-ci, it
would seem prudent that the project encourage if not require pull request
submitters to link to travis-ci and test their builds.


On November 29, 2016 at 16:20:59, James Sirota (jsir...@apache.org) wrote:

We have a really old (and incomplete) coding guidelines document that I'd
like to clean up prior to our graduation. Does anyone have anything in
particular they wanted to add/change about this document? Please post
suggestions to this thread and I will incorporate them

https://cwiki.apache.org/confluence/display/METRON/Development+Guidelines

---
Thank you,

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

Re: [DISCUSS] Coding Guidelines

[Matt Foley]

Apache Hadoop does in fact use checkstyle and the maven checkstyle plugin, see 
https://wiki.apache.org/hadoop/HowToContribute#Contributing_your_work and 
https://github.com/apache/hadoop/blob/trunk/pom.xml .

The checkstyle configuration files are in 
https://github.com/apache/hadoop/tree/trunk/hadoop-build-tools/src/main/resources/checkstyle
(Please note the Hadoop project uses “trunk” rather than “master” as the 
leading edge of their codebase.)
The main file, checkstyle.xml, says:

“Checkstyle configuration for Hadoop that is based on the sun_checks.xml file
that is bundled with Checkstyle and includes checks for:
- the Java Language Specification at
  http://java.sun.com/docs/books/jls/second_edition/html/index.html
- the Sun Code Conventions at http://java.sun.com/docs/codeconv/
- the Javadoc guidelines at
  http://java.sun.com/j2se/javadoc/writingdoccomments/index.html
- the JDK Api documentation http://java.sun.com/j2se/docs/api/index.html
- some best practices”

I’m also +1 on using checkstyle.
--Matt


On 11/30/16, 6:37 AM, "Casey Stella"  wrote:

+1 to what Mike said. This only works IMO if it is enforceable via an
automated task associated with the build.

Are there any common configs that other apache projects use that we might
be able to adapt? (i.e. s the hadoop coding style enforced via checkstyle?)
I'd also suggest that we publish IDE configs for Eclipse and Intellij to
make this sort of thing a bit easier to conform to.


On Wed, Nov 30, 2016 at 9:31 AM, Nick Allen  wrote:

> +1 to checkstyle.  Maybe even tackle that effort project-by-project 
instead
> of one giant PR.  But however it gets done, its got to get done.
>
> On Tue, Nov 29, 2016 at 6:23 PM, Michael Miklavcic <
> michael.miklav...@gmail.com> wrote:
>
> > For 3, I think we should implement checkstyle, perform a one-time mass
> > normalization check-in, and then move forward from there. Right now we
> are
> > at the whim of individual preferences. I don't have a huge issue with
> this
> > in incubation and while features and architecture rapidly change, but we
> > should settle down as we approach maturity.
> >
> > On Nov 29, 2016 4:10 PM, "Matt Foley"  wrote:
> >
> > > 1. I think we need to emphasize good unit testing more.  I guess
> > METRON-37
> > > integrated Cobertura with our build process, but how often does it run
> > and
> > > where is JUnit code coverage reported?
> > >
> > > 2. Regarding the missing Code Style Guidelines, I’ve always thought 
the
> > > Hadoop Style guide was good: short and sweet, and relatively easy to
> > agree
> > > to (for such a deeply “religious” topic :-)  See
> > https://wiki.apache.org/
> > > hadoop/CodeReviewChecklist  The core of it is simply “use Sun's code
> > > conventions < http://www.oracle.com/technetwork/java/javase/
> > > documentation/codeconvtoc-136057.html > except indentation is 2
> spaces,
> > > not 4”.
> > >
> > > 3. Other useful things:
> > > - 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.
> > >
> > > 4. How about mandating Javadoc comments for all public APIs, and
> > > generating Javadocs for release builds?  I’m a big fan of this, and it
> > > isn’t much effort on an on-going basis; we did it in Hadoop.  This is
> how
> > > you scale “self-documenting code” as you get past a few tens of KLOC –
> > good
> > > comments and transparent coding make the code self-documenting at the
> > small
> > > scale, Javadocs make the system self-summarizing at the large scale.
> > > Metron is now about 66 KLOC, just line-counting Java files and
> > subtracting
> > > the ASF License headers (admittedly not excluding other whitespace or
> > > comments – but there aren’t that many comments! :-)  I’d be willing to
> > help
> > > with retroactive cleanup, if there was support for the idea.
> > >
> > > Cheers,
> > > --Matt
> > >
> > >
> > > On 11/29/16, 1:20 PM, "James Sirota"  wrote:
> > >
> > > We have a really old (and incomplete) coding guidelines document
> that
> > > I'd like to clean up prior to our graduation.  Does anyone have
> anything
> > in
> > > particular they wanted to add/change about this document?  Please post
> > > suggestions to this thread and I will incorporate them
> > >
> > > https://cwiki.apache.org/confluence/display/METRON/
> > > 

Re: [DISCUSS] Coding Guidelines

[zeo...@gmail.com]

>From what I can tell, it's not up to date.  For instance the doc mentions
that a committer may never merge their own PR, but I know that has become
somewhat standard practice recently, as long as it has been properly
reviewed.  In addition, the recent transient build issues have been forcing
us into a place where PRs are merged against a failing master and merging a
patch that remove existing tests has been discussed (METRON-597), which are
explicitly forbidden by the guide.  IMO we need to add some exceptions for
scenarios where a failing build is properly identified as a transient issue
and not something systematic.

Jon

On Wed, Nov 30, 2016 at 10:00 AM Otto Fowler 
wrote:

> Are the committing guidelines up to date?   When I stated committing
> someone mentioned that they were not.
>
>
> On November 29, 2016 at 16:20:59, James Sirota (jsir...@apache.org) wrote:
>
> We have a really old (and incomplete) coding guidelines document that I'd
> like to clean up prior to our graduation. Does anyone have anything in
> particular they wanted to add/change about this document? Please post
> suggestions to this thread and I will incorporate them
>
> https://cwiki.apache.org/confluence/display/METRON/Development+Guidelines
>
> ---
> Thank you,
>
> James Sirota
> PPMC- Apache Metron (Incubating)
> jsirota AT apache DOT org
>
-- 

Jon

Sent from my mobile device

Re: [DISCUSS] Coding Guidelines

[Otto Fowler]

Are the committing guidelines up to date?   When I stated committing
someone mentioned that they were not.


On November 29, 2016 at 16:20:59, James Sirota (jsir...@apache.org) wrote:

We have a really old (and incomplete) coding guidelines document that I'd
like to clean up prior to our graduation. Does anyone have anything in
particular they wanted to add/change about this document? Please post
suggestions to this thread and I will incorporate them

https://cwiki.apache.org/confluence/display/METRON/Development+Guidelines

---
Thank you,

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

Re: [DISCUSS] Coding Guidelines

[Nick Allen]

+1 to checkstyle.  Maybe even tackle that effort project-by-project instead
of one giant PR.  But however it gets done, its got to get done.

On Tue, Nov 29, 2016 at 6:23 PM, Michael Miklavcic <
michael.miklav...@gmail.com> wrote:

> For 3, I think we should implement checkstyle, perform a one-time mass
> normalization check-in, and then move forward from there. Right now we are
> at the whim of individual preferences. I don't have a huge issue with this
> in incubation and while features and architecture rapidly change, but we
> should settle down as we approach maturity.
>
> On Nov 29, 2016 4:10 PM, "Matt Foley"  wrote:
>
> > 1. I think we need to emphasize good unit testing more.  I guess
> METRON-37
> > integrated Cobertura with our build process, but how often does it run
> and
> > where is JUnit code coverage reported?
> >
> > 2. Regarding the missing Code Style Guidelines, I’ve always thought the
> > Hadoop Style guide was good: short and sweet, and relatively easy to
> agree
> > to (for such a deeply “religious” topic :-)  See
> https://wiki.apache.org/
> > hadoop/CodeReviewChecklist  The core of it is simply “use Sun's code
> > conventions < http://www.oracle.com/technetwork/java/javase/
> > documentation/codeconvtoc-136057.html > except indentation is 2 spaces,
> > not 4”.
> >
> > 3. Other useful things:
> > - 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.
> >
> > 4. How about mandating Javadoc comments for all public APIs, and
> > generating Javadocs for release builds?  I’m a big fan of this, and it
> > isn’t much effort on an on-going basis; we did it in Hadoop.  This is how
> > you scale “self-documenting code” as you get past a few tens of KLOC –
> good
> > comments and transparent coding make the code self-documenting at the
> small
> > scale, Javadocs make the system self-summarizing at the large scale.
> > Metron is now about 66 KLOC, just line-counting Java files and
> subtracting
> > the ASF License headers (admittedly not excluding other whitespace or
> > comments – but there aren’t that many comments! :-)  I’d be willing to
> help
> > with retroactive cleanup, if there was support for the idea.
> >
> > Cheers,
> > --Matt
> >
> >
> > On 11/29/16, 1:20 PM, "James Sirota"  wrote:
> >
> > We have a really old (and incomplete) coding guidelines document that
> > I'd like to clean up prior to our graduation.  Does anyone have anything
> in
> > particular they wanted to add/change about this document?  Please post
> > suggestions to this thread and I will incorporate them
> >
> > https://cwiki.apache.org/confluence/display/METRON/
> > Development+Guidelines
> >
> > ---
> > Thank you,
> >
> > James Sirota
> > PPMC- Apache Metron (Incubating)
> > jsirota AT apache DOT org
> >
> >
> >
> >
> >
> >
>



-- 
Nick Allen 

Re: [DISCUSS] Coding Guidelines

[Matt Foley]

1. I think we need to emphasize good unit testing more.  I guess METRON-37 
integrated Cobertura with our build process, but how often does it run and 
where is JUnit code coverage reported?

2. Regarding the missing Code Style Guidelines, I’ve always thought the Hadoop 
Style guide was good: short and sweet, and relatively easy to agree to (for 
such a deeply “religious” topic :-)  See 
https://wiki.apache.org/hadoop/CodeReviewChecklist  The core of it is simply 
“use Sun's code conventions < 
http://www.oracle.com/technetwork/java/javase/documentation/codeconvtoc-136057.html
 > except indentation is 2 spaces, not 4”.

3. Other useful things:
- 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.

4. How about mandating Javadoc comments for all public APIs, and generating 
Javadocs for release builds?  I’m a big fan of this, and it isn’t much effort 
on an on-going basis; we did it in Hadoop.  This is how you scale 
“self-documenting code” as you get past a few tens of KLOC – good comments and 
transparent coding make the code self-documenting at the small scale, Javadocs 
make the system self-summarizing at the large scale.  Metron is now about 66 
KLOC, just line-counting Java files and subtracting the ASF License headers 
(admittedly not excluding other whitespace or comments – but there aren’t that 
many comments! :-)  I’d be willing to help with retroactive cleanup, if there 
was support for the idea.

Cheers,
--Matt


On 11/29/16, 1:20 PM, "James Sirota"  wrote:

We have a really old (and incomplete) coding guidelines document that I'd 
like to clean up prior to our graduation.  Does anyone have anything in 
particular they wanted to add/change about this document?  Please post 
suggestions to this thread and I will incorporate them

https://cwiki.apache.org/confluence/display/METRON/Development+Guidelines

--- 
Thank you,

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

[DISCUSS] Coding Guidelines

[James Sirota]

We have a really old (and incomplete) coding guidelines document that I'd like 
to clean up prior to our graduation.  Does anyone have anything in particular 
they wanted to add/change about this document?  Please post suggestions to this 
thread and I will incorporate them

https://cwiki.apache.org/confluence/display/METRON/Development+Guidelines

--- 
Thank you,

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