+1 Thanks for the updates to the documentation and testing sections. -Kyle
On Wed, Dec 21, 2016 at 3:17 PM, Otto Fowler <[email protected]> wrote: > +1 > > > > On December 21, 2016 at 14:56:06, Michael Miklavcic ( > [email protected]) wrote: > > Works for me also. > > On Wed, Dec 21, 2016 at 12:38 PM, Matt Foley <[email protected]> wrote: > > > Works for me, thanks. > > > > On 12/21/16, 11:21 AM, "Casey Stella" <[email protected]> wrote: > > > > Sure, how about making it generic to "a deployed cluster"? > > > > On Wed, Dec 21, 2016 at 2:20 PM, Matt Foley <[email protected]> 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" < > > [email protected]> > > > 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 < > > [email protected]> > > > 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 < > > [email protected]> > > > 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 < > > [email protected]> > > > > wrote: > > > > > > > > > >> Yeah, I +1 the notion of thorough automated tests. > > > > >> > > > > >> On Tue, Dec 20, 2016 at 4:36 PM, Matt Foley < > > [email protected]> > > > wrote: > > > > >> > > > > >>> Hard to mark diffs in text-only mode :-) My proposed > > change is: > > > > >>> > > > > >>> >> All merged patches will be reviewed with the > > expectation that > > > > >>> thorough automated tests shall be provided and are > > consistent > > > with … > > > > >>> > > > > >>> ^^^^^^^^ > > > > >>> ^^^^^^^^^^^^^^ > > > > >>> Added word “thorough” and changed “exist” to “shall be > > provided”. > > > > >>> Thanks, > > > > >>> --Matt > > > > >>> > > > > >>> On 12/20/16, 1:22 PM, "James Sirota" <[email protected]> > > wrote: > > > > >>> > > > > >>> Hi Matt, thats already in there. See last bullet point > > of 2.6 > > > > >>> > > > > >>> 20.12.2016, 14:14, "Matt Foley" <[email protected]>: > > > > >>> > If we aren't going to require 100% test coverage for > > new > > > code, > > > > >>> then we should at least say "thorough" automated tests, in > > the > > > last > > > > bullet > > > > >>> of 2.6. And it should be a mandate not an assumption: > > > > >>> > > > > > >>> > All merged patches will be reviewed with the > > expectation > > > that > > > > >>> thorough automated tests shall be provided and are > > consistent > > > with > > > > project > > > > >>> testing methodology and practices, and cover the > > appropriate > > > cases ( > > > > see > > > > >>> reviewers guide ) > > > > >>> > > > > > >>> > IMO, > > > > >>> > --Matt > > > > >>> > > > > > >>> > On 12/20/16, 12:51 PM, "James Sirota" < > > [email protected]> > > > > wrote: > > > > >>> > > > > > >>> > Good feedback. Here is the next iteration that > > > accounts for > > > > >>> your suggestions: > > > > >>> > https://cwiki.apache.org/ > > confluence/pages/viewpage. > > > action?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 [email protected] > > and > > > someone > > > > >>> will either give you permission or assign a JIRA to you > > > > >>> > If you are introducing a completely new feature > > or API > > > it is > > > > a > > > > >>> good idea to start a discussion and get consensus on the > > basic > > > design > > > > >>> first. Larger changes should be discussed on the dev boards > > > before > > > > >>> submission. > > > > >>> > New features and significant bug fixes should be > > > documented > > > > in > > > > >>> the JIRA and appropriate architecture diagrams should be > > > attached. > > > > Major > > > > >>> features may require a vote. > > > > >>> > Note that if the change is related to user-facing > > > protocols / > > > > >>> interface / configs, etc, you need to make the > > corresponding > > > change on > > > > the > > > > >>> documentation as well. > > > > >>> > Craft a pull request following the guidelines in > > > Section 2 of > > > > >>> this document > > > > >>> > Pull requests should be small to facilitate > > easier > > > review. > > > > >>> Studies have shown that review quality falls off as patch > > size > > > grows. > > > > >>> Sometimes this will result in many small PRs to land a > > single > > > large > > > > feature. > > > > >>> > People will review and comment on your pull > > request. > > > It is > > > > our > > > > >>> job to follow up on pull requests in a timely fashion. > > > > >>> > Once the pull request is merged the person doing > > the > > > merge > > > > >>> (committer) should manually close the corresponding JIRA. > > > > >>> > 1.2 Reviewing and merging patches > > > > >>> > Everyone is encouraged to review open pull > > requests. > > > We only > > > > >>> ask that you try and think carefully, ask questions and are > > > excellent > > > > to > > > > >>> one another. Code review is our opportunity to share > > knowledge, > > > design > > > > >>> ideas and make friends. > > > > >>> > When reviewing a patch try to keep each of these > > > concepts in > > > > >>> mind: > > > > >>> > > > > > >>> > Is the proposed change being made in the correct > > > place? Is it > > > > >>> a fix in a backend when it should be in the primitives? In > > Kafka > > > vs > > > > Storm? > > > > >>> > What is the change being proposed? Is it based on > > > Community > > > > >>> recognized issues? > > > > >>> > Do we want this feature or is the bug they’re > > fixing > > > really a > > > > >>> bug? > > > > >>> > Does the change do what the author claims? > > > > >>> > Are there sufficient tests? > > > > >>> > Has it been documented? > > > > >>> > Will this change introduce new bugs? > > > > >>> > > > > > >>> > 2. Implementation > > > > >>> > > > > > >>> > 2.1 Grammar and style > > > > >>> > These are small things that are not caught by the > > > automated > > > > >>> style checkers. > > > > >>> > Does a variable need a better name? > > > > >>> > Should this be a keyword argument? > > > > >>> > In a PR, maintain the existing style of the file. > > > > >>> > Don’t combine code changes with lots of edits of > > > whitespace > > > > or > > > > >>> comments; it makes code review too difficult. It’s okay to > > fix an > > > > >>> occasional comment or indenting, but if wholesale comment > > or > > > whitespace > > > > >>> changes are needed, make them a separate PR. > > > > >>> > Use the checkstyle plugin in Maven to verify > > that your > > > PR > > > > >>> conforms to our style > > > > >>> > 2.2 Code Style > > > > >>> > Follow the Sun Code Conventions outlined here: > > > > >>> http://www.oracle.com/technetwork/java/codeconvtoc- > > 136057.html > > > > >>> > except that indents are 2 spaces instead of 4 > > > > >>> > 2.3 Coding Standards > > > > >>> > Implementation matches what the documentation > > says > > > > >>> > Logger name is effectively the result of > > > Class.getName() > > > > >>> > Class & member access - as restricted as it can > > be > > > (subject > > > > to > > > > >>> testing requirements) > > > > >>> > Appropriate NullPointerException and > > > IllegalArgumentException > > > > >>> argument checks > > > > >>> > Asserts - verify they should always be true > > > > >>> > Look for accidental propagation of exceptions > > > > >>> > Look for unanticipated runtime exceptions > > > > >>> > Try-finally used as necessary to restore > > consistent > > > state > > > > >>> > Logging levels conform to Log4j levels > > > > >>> > Possible deadlocks - look for inconsistent > > locking > > > order > > > > >>> > Race conditions - look for missing or inadequate > > > > >>> synchronization > > > > >>> > Consistent synchronization - always locking the > > same > > > > object(s) > > > > >>> > Look for synchronization or documentation saying > > > there's no > > > > >>> synchronization > > > > >>> > Look for possible performance problems > > > > >>> > Look at boundary conditions for problems > > > > >>> > Configuration entries are retrieved/set via > > > setter/getter > > > > >>> methods > > > > >>> > Implementation details do NOT leak into > > interfaces > > > > >>> > Variables and arguments should be interfaces > > where > > > possible > > > > >>> > If equals is overridden then hashCode is > > overridden > > > (and vice > > > > >>> versa) > > > > >>> > Objects are checked (instanceof) for appropriate > > type > > > before > > > > >>> casting (use generics if possible) > > > > >>> > Public API changes have been publicly discussed > > > > >>> > Use of static member variables should be used > > with > > > caution > > > > >>> especially in Map/reduce tasks due to the JVM reuse feature > > > > >>> > 2.4 Documentation > > > > >>> > > > > > >>> > Code-Level Documentation > > > > >>> > Self-documenting code (variable, method, class) > > has a > > > clear > > > > >>> semantic name > > > > >>> > Accurate, sufficient for developers to code > > against > > > > >>> > Follows standard Javadoc conventions > > > > >>> > Loggers and logging levels covered if they do not > > > follow our > > > > >>> conventions (see below) > > > > >>> > System properties, configuration options, and > > resources > > > > covered > > > > >>> > Illegal arguments are properly documented as > > > appropriate > > > > >>> > Package and overview Javadoc are updated as > > appropriate > > > > >>> > Javadoc comments are mandatory for all public > > APIs > > > > >>> > Generate Javadocs for release builds > > > > >>> > > > > > >>> > Feature-level documentation - should be version > > > controlled in > > > > >>> github in README files. > > > > >>> > Accurate description of the feature > > > > >>> > Sample configuration and deployment options > > > > >>> > Sample usage scenarios > > > > >>> > > > > > >>> > High-Level Design documentation - architecture > > > description > > > > and > > > > >>> diagrams should be a part of a wiki entry. > > > > >>> > Provide diagrams/charts where appropriate. > > Visuals are > > > always > > > > >>> welcome > > > > >>> > Provide purpose of the feature/module and why it > > exists > > > > within > > > > >>> the project > > > > >>> > Describe system flows through the feature/module > > where > > > > >>> appropriate > > > > >>> > Describe how the feature/module interfaces with > > the > > > rest of > > > > >>> the system > > > > >>> > Describe appropriate usages scenarios and use > > cases > > > > >>> > > > > > >>> > Tutorials - system-level tutorials and use cases > > > should also > > > > >>> be kept as wiki entries. > > > > >>> > Add to the Metron reference application > > documentation > > > for > > > > each > > > > >>> additional major feature > > > > >>> > If appropriate, publish a tutorials blog on the > > Wiki to > > > > >>> highlight usage scenarios and apply them to the real world > > use > > > cases > > > > >>> > 2.5 Tests > > > > >>> > Unit tests exist for bug fixes and new features, > > or a > > > > >>> rationale is given in JIRA for why there is no test > > > > >>> > Unit tests do not write any temporary files to > > /tmp > > > > (instead, > > > > >>> the tests should write to the location specified by the > > > test.build.data > > > > >>> system property) > > > > >>> > > > > > >>> > 2.6 Merge requirements > > > > >>> > Because Metron is so complex, and the > > implications of > > > getting > > > > >>> it wrong so devastating, Metron has a strict merge policy > > for > > > > committers: > > > > >>> > Patches must never be pushed directly to master, > > all > > > changes > > > > >>> (even the most trivial typo fixes!) must be submitted as a > > pull > > > > request. > > > > >>> > A committer may merge their own pull request, > > but only > > > after > > > > a > > > > >>> second reviewer has given it a +1. A qualified reviewer is > > a > > > Metron > > > > >>> committer or PPMC member. > > > > >>> > A non-committer may ask the reviewer to merge > > their > > > pull > > > > >>> request or alternatively post to the Metron dev board to > > get > > > another > > > > >>> committer to merge the PR if the reviewer is not available. > > > > >>> > There should be at least one independent party > > besides > > > the > > > > >>> committer that have reviewed the patch before merge. > > > > >>> > A patch that breaks tests, or introduces > > regressions by > > > > >>> changing or removing existing tests should not be merged. > > Tests > > > must > > > > always > > > > >>> be passing on master. This implies that the tests have > > been run. > > > > >>> > All pull request submitters must link to > > travis-ci > > > > >>> > If somehow the tests get into a failing state on > > > master (such > > > > >>> as by a backwards incompatible release of a dependency) no > > pull > > > > requests > > > > >>> may be merged until this is rectified. > > > > >>> > All merged patches will be reviewed with the > > > expectation that > > > > >>> automated tests exist and are consistent with project > > testing > > > > methodology > > > > >>> and practices, and cover the appropriate cases ( see > > reviewers > > > guide ) > > > > >>> > > > > > >>> > The purpose of these policies is to minimize the > > > chances we > > > > >>> merge a change that has unintended consequences. > > > > >>> > > > > > >>> > 3. JIRA > > > > >>> > The Incompatible change flag on the issue's JIRA > > is set > > > > >>> appropriately for this patch > > > > >>> > For incompatible changes, major > > features/improvements, > > > and > > > > >>> other release notable issues, the Release Note field has a > > > sufficient > > > > >>> comment > > > > >>> > > > > > >>> > 20.12.2016, 13:18, "Otto Fowler" < > > > [email protected]>: > > > > >>> > > "The purpose of these policies is to minimize > > the > > > chances > > > > we > > > > >>> merge a change > > > > >>> > > that jeopardizes has unintended consequences." > > > > >>> > > > > > > >>> > > remove jeopardizes? > > > > >>> > > > > > > >>> > > On December 20, 2016 at 13:25:35, James Sirota > > ( > > > > >>> [email protected]) wrote: > > > > >>> > > > > > > >>> > > I incorporated the changes. This is what the > > doc > > > looks like > > > > >>> now: > > > > >>> > > https://cwiki.apache.org/ > > confluence/pages/viewpage. > > > > action?pa > > > > >>> geId=61332235 > > > > >>> > > > > > > >>> > > As an open source project, Metron welcomes > > > contributions of > > > > >>> all forms. The > > > > >>> > > sections below will help you get started. > > > > >>> > > 1. How To Contribute > > > > >>> > > We are always very happy to have contributions, > > > whether for > > > > >>> trivial > > > > >>> > > cleanups, little additions or big new features. > > > > >>> > > If you don't know Java or Scala you can still > > > contribute to > > > > >>> the project. We > > > > >>> > > strongly value documentation and gladly accept > > > improvements > > > > >>> to the > > > > >>> > > documentation. > > > > >>> > > 1.1 Contributing A Code Change > > > > >>> > > To submit a change for inclusion, please do the > > > following: > > > > >>> > > If there is not already a JIRA associated with > > your > > > pull > > > > >>> request, create > > > > >>> > > it, assign it to yourself, and start progress > > > > >>> > > If there is a JIRA already created for your > > change > > > then > > > > >>> assign it to > > > > >>> > > yourself and start progress > > > > >>> > > If you don't have access to JIRA or can't > > assign an > > > issue > > > > to > > > > >>> yourself, > > > > >>> > > please message [email protected] > > and > > > someone > > > > >>> will give you > > > > >>> > > permission > > > > >>> > > If you are introducing a completely new > > feature or > > > API it > > > > is > > > > >>> a good idea to > > > > >>> > > start a discussion and get consensus on the > > basic > > > design > > > > >>> first. Larger > > > > >>> > > changes should be discussed on the dev boards > > before > > > > >>> submission. > > > > >>> > > New features and significant bug fixes should > > be > > > documented > > > > >>> in the JIRA and > > > > >>> > > appropriate architecture diagrams should be > > > attached. Major > > > > >>> features may > > > > >>> > > require a vote. > > > > >>> > > Note that if the change is related to > > user-facing > > > protocols > > > > >>> / interface / > > > > >>> > > configs, etc, you need to make the > > corresponding > > > change on > > > > >>> the > > > > >>> > > documentation as well. > > > > >>> > > Craft a pull request following the guidelines > > in > > > Section 2 > > > > >>> of this document > > > > >>> > > Pull requests should be small to facilitate > > easier > > > review. > > > > >>> Studies have > > > > >>> > > shown that review quality falls off as patch > > size > > > grows. > > > > >>> Sometimes this > > > > >>> > > will result in many small PRs to land a single > > large > > > > feature. > > > > >>> > > People will review and comment on your pull > > request. > > > It is > > > > >>> our job to > > > > >>> > > follow up on pull requests in a timely fashion. > > > > >>> > > Once the pull request is merged, manually > > close the > > > > >>> corresponding JIRA > > > > >>> > > 1.2 Reviewing and merging patches > > > > >>> > > Everyone is encouraged to review open pull > > requests. > > > We > > > > only > > > > >>> ask that you > > > > >>> > > try and think carefully, ask questions and are > > > excellent to > > > > >>> one another. > > > > >>> > > Code review is our opportunity to share > > knowledge, > > > design > > > > >>> ideas and make > > > > >>> > > friends. > > > > >>> > > When reviewing a patch try to keep each of > > these > > > concepts > > > > in > > > > >>> mind: > > > > >>> > > > > > > >>> > > Is the proposed change being made in the > > correct > > > place? Is > > > > >>> it a fix in a > > > > >>> > > backend when it should be in the primitives? In > > > Kafka vs > > > > >>> Storm? > > > > >>> > > What is the change being proposed? Is it based > > on > > > Community > > > > >>> recognized > > > > >>> > > issues? > > > > >>> > > Do we want this feature or is the bug they’re > > fixing > > > really > > > > >>> a bug? > > > > >>> > > Does the change do what the author claims? > > > > >>> > > Are there sufficient tests? > > > > >>> > > Has it been documented? > > > > >>> > > Will this change introduce new bugs? > > > > >>> > > > > > > >>> > > 2. Implementation > > > > >>> > > > > > > >>> > > 2.1 Grammar and style > > > > >>> > > These are small things that are not caught by > > the > > > automated > > > > >>> style checkers. > > > > >>> > > Does a variable need a better name? > > > > >>> > > Should this be a keyword argument? > > > > >>> > > In a PR, maintain the existing style of the > > file. > > > > >>> > > Don’t combine code changes with lots of edits > > of > > > whitespace > > > > >>> or comments; it > > > > >>> > > makes code review too difficult. It’s okay to > > fix an > > > > >>> occasional comment or > > > > >>> > > indenting, but if wholesale comment or > > whitespace > > > changes > > > > >>> are needed, make > > > > >>> > > them a separate PR. > > > > >>> > > Use the checkstyle plugin in Maven to verify > > that > > > your PR > > > > >>> conforms to our > > > > >>> > > style > > > > >>> > > 2.2 Code Style > > > > >>> > > Follow the Sun Code Conventions outlined here: > > > > >>> > > http://www.oracle.com/ > > technetwork/java/codeconvtoc- > > > > 136057.ht > > > > >>> ml > > > > >>> > > except that indents are 2 spaces instead of 4 > > > > >>> > > 2.3 Coding Standards > > > > >>> > > Implementation matches what the documentation > > says > > > > >>> > > Logger name is effectively the result of > > > Class.getName() > > > > >>> > > Class & member access - as restricted as it > > can be > > > (subject > > > > >>> to testing > > > > >>> > > requirements) > > > > >>> > > Appropriate NullPointerException and > > > > >>> IllegalArgumentException argument > > > > >>> > > checks > > > > >>> > > Asserts - verify they should always be true > > > > >>> > > Look for accidental propagation of exceptions > > > > >>> > > Look for unanticipated runtime exceptions > > > > >>> > > Try-finally used as necessary to restore > > consistent > > > state > > > > >>> > > Logging levels conform to Log4j levels > > > > >>> > > Possible deadlocks - look for inconsistent > > locking > > > order > > > > >>> > > Race conditions - look for missing or > > inadequate > > > > >>> synchronization > > > > >>> > > Consistent synchronization - always locking > > the same > > > > >>> object(s) > > > > >>> > > Look for synchronization or documentation > > saying > > > there's no > > > > >>> synchronization > > > > >>> > > Look for possible performance problems > > > > >>> > > Look at boundary conditions for problems > > > > >>> > > Configuration entries are retrieved/set via > > > setter/getter > > > > >>> methods > > > > >>> > > Implementation details do NOT leak into > > interfaces > > > > >>> > > Variables and arguments should be interfaces > > where > > > possible > > > > >>> > > If equals is overridden then hashCode is > > overridden > > > (and > > > > >>> vice versa) > > > > >>> > > Objects are checked (instanceof) for > > appropriate type > > > > before > > > > >>> casting (use > > > > >>> > > generics if possible) > > > > >>> > > Public API changes have been publicly discussed > > > > >>> > > Use of static member variables should be used > > with > > > caution > > > > >>> especially in > > > > >>> > > Map/reduce tasks due to the JVM reuse feature > > > > >>> > > 2.4 Documentation > > > > >>> > > > > > > >>> > > Code-Level Documentation > > > > >>> > > Self-documenting code (variable, method, > > class) has > > > a clear > > > > >>> semantic name > > > > >>> > > Accurate, sufficient for developers to code > > against > > > > >>> > > Follows standard Javadoc conventions > > > > >>> > > Loggers and logging levels covered if they do > > not > > > follow > > > > our > > > > >>> conventions > > > > >>> > > (see below) > > > > >>> > > System properties, configuration options, and > > > resources > > > > >>> covered > > > > >>> > > Illegal arguments are properly documented as > > > appropriate > > > > >>> > > Package and overview Javadoc are updated as > > > appropriate > > > > >>> > > Javadoc comments are mandatory for all public > > APIs > > > > >>> > > Generate Javadocs for release builds > > > > >>> > > > > > > >>> > > Feature-level documentation - should be version > > > controlled > > > > >>> in github in > > > > >>> > > README files. > > > > >>> > > Accurate description of the feature > > > > >>> > > Sample configuration and deployment options > > > > >>> > > Sample usage scenarios > > > > >>> > > > > > > >>> > > High-Level Design documentation - architecture > > > description > > > > >>> and diagrams > > > > >>> > > should be a part of a wiki entry. > > > > >>> > > Provide diagrams/charts where appropriate. > > Visuals > > > are > > > > >>> always welcome > > > > >>> > > Provide purpose of the feature/module and why > > it > > > exists > > > > >>> within the project > > > > >>> > > Describe system flows through the > > feature/module > > > where > > > > >>> appropriate > > > > >>> > > Describe how the feature/module interfaces > > with the > > > rest of > > > > >>> the system > > > > >>> > > Describe appropriate usages scenarios and use > > cases > > > > >>> > > > > > > >>> > > Tutorials - system-level tutorials and use > > cases > > > should > > > > also > > > > >>> be kept as > > > > >>> > > wiki entries. > > > > >>> > > Add to the Metron reference application > > > documentation for > > > > >>> each additional > > > > >>> > > major feature > > > > >>> > > If appropriate, publish a tutorials blog on > > the Wiki > > > to > > > > >>> highlight usage > > > > >>> > > scenarios and apply them to the real world use > > cases > > > > >>> > > 2.5 Tests > > > > >>> > > Unit tests exist for bug fixes and new > > features, or a > > > > >>> rationale is given in > > > > >>> > > JIRA for why there is no test > > > > >>> > > Unit tests do not write any temporary files to > > /tmp > > > > >>> (instead, the tests > > > > >>> > > should write to the location specified by the > > > > >>> test.build.data system > > > > >>> > > property) > > > > >>> > > > > > > >>> > > 2.6 Merge requirements > > > > >>> > > Because Metron is so complex, and the > > implications of > > > > >>> getting it wrong so > > > > >>> > > devastating, Metron has a strict merge policy > > for > > > > committers: > > > > >>> > > Patches must never be pushed directly to > > master, all > > > > changes > > > > >>> (even the most > > > > >>> > > trivial typo fixes!) must be submitted as a > > pull > > > request. > > > > >>> > > A committer may merge their own pull request, > > but > > > only > > > > after > > > > >>> a second > > > > >>> > > reviewer has given it a +1. A qualified > > reviewer is a > > > > Metron > > > > >>> committer or > > > > >>> > > PPMC member. > > > > >>> > > A non-committer may ask the reviewer to merge > > their > > > pull > > > > >>> request or > > > > >>> > > alternatively post to the Metron dev board to > > get > > > another > > > > >>> committer to > > > > >>> > > merge the PR if the reviewer is not available. > > > > >>> > > There should be at least one independent party > > > besides the > > > > >>> committer that > > > > >>> > > have reviewed the patch before merge. > > > > >>> > > A patch that breaks tests, or introduces > > regressions > > > by > > > > >>> changing or > > > > >>> > > removing existing tests should not be merged. > > Tests > > > must > > > > >>> always be passing > > > > >>> > > on master. This implies that the tests have > > been run. > > > > >>> > > All pull request submitters must link to > > travis-ci > > > > >>> > > If somehow the tests get into a failing state > > on > > > master > > > > >>> (such as by a > > > > >>> > > backwards incompatible release of a > > dependency) no > > > pull > > > > >>> requests may be > > > > >>> > > merged until this is rectified. > > > > >>> > > All merged patches will be reviewed with the > > > expectation > > > > >>> that automated > > > > >>> > > tests exist and are consistent with project > > testing > > > > >>> methodology and > > > > >>> > > practices, and cover the appropriate cases ( > > see > > > reviewers > > > > >>> guide ) > > > > >>> > > > > > > >>> > > The purpose of these policies is to minimize > > the > > > chances we > > > > >>> merge a change > > > > >>> > > that jeopardizes has unintended consequences. > > > > >>> > > > > > > >>> > > 3. JIRA > > > > >>> > > The Incompatible change flag on the issue's > > JIRA is > > > set > > > > >>> appropriately for > > > > >>> > > this patch > > > > >>> > > For incompatible changes, major > > > features/improvements, and > > > > >>> other release > > > > >>> > > notable issues, the Release Note field has a > > > sufficient > > > > >>> comment > > > > >>> > > > > > > >>> > > 20.12.2016, 09:42, "[email protected]" < > > > [email protected]>: > > > > >>> > >> I don't have enough experience on the > > Java/Javadoc > > > side to > > > > >>> make a > > > > >>> > > > > > > >>> > > specific > > > > >>> > >> suggestion, but with other languages I've used > > > Sphinx and > > > > >>> Doxygen with > > > > >>> > >> great results. > > > > >>> > >> > > > > >>> > >> Jon > > > > >>> > >> > > > > >>> > >> On Tue, Dec 20, 2016 at 11:29 AM Michael > > Miklavcic < > > > > >>> > >> [email protected]> wrote: > > > > >>> > >> > > > > >>> > >>> Were you thinking javadoc or something more? > > I > > > wouldn't > > > > >>> mind seeing us > > > > >>> > >>> produce a javadoc site, if we aren't already > > doing > > > so. > > > > >>> > >>> > > > > >>> > >>> On Dec 20, 2016 9:25 AM, "[email protected]" > > < > > > > >>> [email protected]> wrote: > > > > >>> > >>> > > > > >>> > >>> > Regarding documentation - while I'm not a > > huge > > > fan of > > > > >>> that approach > > > > >>> > > > > > > >>> > > (I > > > > >>> > >>> > would prefer to see documentation > > generated from > > > the > > > > >>> code), I think > > > > >>> > > > > > > >>> > > it > > > > >>> > >>> > could work in the short term. Having that > > > outlined both > > > > >>> in the coding > > > > >>> > >>> > guidelines and on the wiki would be > > important. > > > > >>> > >>> > > > > > >>> > >>> > I agree with the comments about author != > > > committer, > > > > and > > > > >>> 100% code > > > > >>> > >>> > coverage. > > > > >>> > >>> > > > > > >>> > >>> > Jon > > > > >>> > >>> > > > > > >>> > >>> > On Tue, Dec 20, 2016 at 11:10 AM James > > Sirota < > > > > >>> [email protected]> > > > > >>> > >>> wrote: > > > > >>> > >>> > > > > > >>> > >>> > > In my view the lower-level documentation > > that > > > should > > > > >>> be source > > > > >>> > >>> controlled > > > > >>> > >>> > > with the code belongs on github and then > > use > > > case > > > > >>> documentation and > > > > >>> > >>> > > top-level architecture diagrams belong > > on the > > > wiki. > > > > >>> What do you > > > > >>> > > > > > > >>> > > think? > > > > >>> > >>> > > > > > > >>> > >>> > > I think if the author is not a committer > > and > > > can't > > > > >>> merge then the > > > > >>> > >>> > reviewer > > > > >>> > >>> > > should probably merge or the PR > > originator > > > should > > > > ping > > > > >>> the dev > > > > >>> > > > > > > >>> > > board to > > > > >>> > >>> > get > > > > >>> > >>> > > someone to merge the PR in. Does that > > seem > > > reasonable > > > > >>> to everyone? > > > > >>> > >>> > > > > > > >>> > >>> > > 18.12.2016, 13:10, "Kyle Richardson" < > > > > >>> [email protected]>: > > > > >>> > >>> > > > Couple of questions/comments: > > > > >>> > >>> > > > > > > > >>> > >>> > > > In 2.4, we talk about Javadoc and code > > > comments but > > > > >>> not too much > > > > >>> > >>> about > > > > >>> > >>> > > the > > > > >>> > >>> > > > user documentation. Should we, > > possibly in a > > > > section > > > > >>> 4, give some > > > > >>> > >>> > > > recommendations on what should go into > > the > > > README > > > > >>> files versus on > > > > >>> > > > > > > >>> > > the > > > > >>> > >>> > > wiki? > > > > >>> > >>> > > > This could also help the reviewer know > > if the > > > > change > > > > >>> is > > > > >>> > > > > > > >>> > > documented > > > > >>> > >>> > > > sufficiently. > > > > >>> > >>> > > > > > > > >>> > >>> > > > In 2.6, we say that 1 qualified > > reviewer > > > (Apache > > > > >>> committer or > > > > >>> > > > > > > >>> > > PPMC > > > > >>> > >>> > > member) > > > > >>> > >>> > > > other than the author of the PR must > > have > > > given it > > > > a > > > > >>> +1. In the > > > > >>> > > > > > > >>> > > case > > > > >>> > >>> > > where > > > > >>> > >>> > > > the author is not a committer (who > > could > > > merge > > > > their > > > > >>> own PR), > > > > >>> > > > > > > >>> > > should > > > > >>> > >>> we > > > > >>> > >>> > > > state that the reviewer will be > > responsible > > > for the > > > > >>> merge? > > > > >>> > >>> > > > > > > > >>> > >>> > > > -Kyle > > > > >>> > >>> > > > > > > > >>> > >>> > > > On Fri, Dec 16, 2016 at 6:39 PM, James > > > Sirota < > > > > >>> [email protected]> > > > > >>> > > > > > > >>> > >>> > > wrote: > > > > >>> > >>> > > > > > > > >>> > >>> > > >> Lets move this back to the discuss > > thread > > > since > > > > >>> it's still > > > > >>> > >>> generating > > > > >>> > >>> > > that > > > > >>> > >>> > > >> many comments. Please post all your > > > feedback and I > > > > >>> will > > > > >>> > > > > > > >>> > > incorporate > > > > >>> > >>> > it > > > > >>> > >>> > > and > > > > >>> > >>> > > >> put it back to a vote. > > > > >>> > >>> > > >> > > > > >>> > >>> > > >> Thanks, > > > > >>> > >>> > > >> James > > > > >>> > >>> > > >> > > > > >>> > >>> > > >> 16.12.2016, 16:12, "Matt Foley" < > > > [email protected] > > > > >: > > > > >>> > >>> > > >> > +1 > > > > >>> > >>> > > >> > > > > > >>> > >>> > > >> > In 2.2 (follow Sun guidelines), do > > you > > > want to > > > > >>> add the > > > > >>> > > > > > > >>> > > notation > > > > >>> > >>> > > “except > > > > >>> > >>> > > >> that indents are 2 spaces instead of > > 4”, as > > > Hadoop > > > > >>> does? Or does > > > > >>> > >>> the > > > > >>> > >>> > > Metron > > > > >>> > >>> > > >> community like 4-space indents? I see > > both > > > in the > > > > >>> Metron code. > > > > >>> > >>> > > >> > > > > > >>> > >>> > > >> > My +1 holds in either case. > > > > >>> > >>> > > >> > --Matt > > > > >>> > >>> > > >> > > > > > >>> > >>> > > >> > On 12/16/16, 9:34 AM, "James > > Sirota" < > > > > >>> [email protected]> > > > > >>> > > > > > > >>> > > wrote: > > > > >>> > >>> > > >> > > > > > >>> > >>> > > >> > I incorporated the changes to the > > coding > > > > >>> guidelines from our > > > > >>> > >>> > discuss > > > > >>> > >>> > > >> thread. I'd like to get them voted on > > to > > > make them > > > > >>> official. > > > > >>> > >>> > > >> > > > > > >>> > >>> > > >> > https://cwiki.apache.org/confl > > > > >>> uence/pages/viewpage. > > > > >>> > >>> > > >> action?pageId=61332235 > > > > >>> > >>> > > >> > > > > > >>> > >>> > > >> > Please vote +1, -1, 0 > > > > >>> > >>> > > >> > > > > > >>> > >>> > > >> > The vote will be open for 72 hours. > > > > >>> > >>> > > >> > > > > > >>> > >>> > > >> > ------------------- > > > > >>> > >>> > > >> > Thank you, > > > > >>> > >>> > > >> > > > > > >>> > >>> > > >> > James Sirota > > > > >>> > >>> > > >> > PPMC- Apache Metron (Incubating) > > > > >>> > >>> > > >> > jsirota AT apache DOT org > > > > >>> > >>> > > >> > > > > >>> > >>> > > >> ------------------- > > > > >>> > >>> > > >> Thank you, > > > > >>> > >>> > > >> > > > > >>> > >>> > > >> James Sirota > > > > >>> > >>> > > >> PPMC- Apache Metron (Incubating) > > > > >>> > >>> > > >> jsirota AT apache DOT org > > > > >>> > >>> > > > > > > >>> > >>> > > ------------------- > > > > >>> > >>> > > Thank you, > > > > >>> > >>> > > > > > > >>> > >>> > > James Sirota > > > > >>> > >>> > > PPMC- Apache Metron (Incubating) > > > > >>> > >>> > > jsirota AT apache DOT org > > > > >>> > >>> > > > > > > >>> > >>> > -- > > > > >>> > >>> > > > > > >>> > >>> > Jon > > > > >>> > >>> > > > > > >>> > >>> > Sent from my mobile device > > > > >>> > >>> > > > > > >>> > >> -- > > > > >>> > >> > > > > >>> > >> Jon > > > > >>> > >> > > > > >>> > >> Sent from my mobile device > > > > >>> > > > > > > >>> > > ------------------- > > > > >>> > > Thank you, > > > > >>> > > > > > > >>> > > James Sirota > > > > >>> > > PPMC- Apache Metron (Incubating) > > > > >>> > > jsirota AT apache DOT org > > > > >>> > > > > > >>> > ------------------- > > > > >>> > Thank you, > > > > >>> > > > > > >>> > James Sirota > > > > >>> > PPMC- Apache Metron (Incubating) > > > > >>> > jsirota AT apache DOT org > > > > >>> > > > > >>> ------------------- > > > > >>> Thank you, > > > > >>> > > > > >>> James Sirota > > > > >>> PPMC- Apache Metron (Incubating) > > > > >>> jsirota AT apache DOT org > > > > >>> > > > > >>> > > > > >>> > > > > >>> > > > > >>> > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
