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" <[email protected]>: > +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 >> > > > >>> >> > > > >>> >> > > > >>> >> > > > >>> >> > > > >>> >> > > > >> >> > > > > >> > > > >> > > >> > > >> > > >> > > >> > > >> > >> > >> > >> > >> > ------------------- Thank you, James Sirota PPMC- Apache Metron (Incubating) jsirota AT apache DOT org
