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