On Fri, Feb 10, 2017 at 8:45 AM, Dan Halperin <dhalp...@google.com.invalid> wrote:
> On Fri, Feb 10, 2017 at 7:42 AM, Kenneth Knowles <k...@google.com.invalid> > wrote: > > > On Feb 10, 2017 07:36, "Dan Halperin" <dhalp...@google.com.invalid> > wrote: > > > > Before we added checkstyle it was under a minute. Now it's over five? > > That's awful IMO > > > > > > Checkstyle didn't cause all that, did it? > > > > The "5 minutes" was going with Aviem's numbers after this change. But yes, > Checkstyle alone substantially (>+50%) the time from what it was previously > to adding it back to the default build. Just to check, are we doing parallel builds? > > Noting that findbugs takes quite a lot more time. Javadoc and jar are the > > other two slow ones. > > > > RAT is fast. But it has very poor error messages, so we wouldn't want a > new > > contributor trying to figure out what is going on without our help. > > > > There is a larger philosophical issue here: is there a point of Jenkins > precommit testing? Why not just make `mvn install` run everything that > Jenkins does? For that matter, why don't committers just push directly to > master? Wouldn't that make everyone's life easier? > > I'd argue that's not true. > > 1. Developer productivity -- Jenkins should run many more checks than > developers do. Especially time-, resource-, or setup- intensive tasks. > 2. Automated enforcement -- Jenkins is better at running the right commands > than we are. > 3. Lower the barrier to entry -- individual developers need not have a > running Spark/Flink/Apex/Dataflow setup in order to contribute code. > 4. Focus on the user -- someone checking out the code and using it for the > first time does not care whether the code style checks or has the right > licenses -- that should have been enforced by the Beam team before > committing. > > We should be *very* choosy about what we enforce on every developer every > time they go to compile. I probably compile Beam 50x-100x a day. Literally, > the extra minutes you want to add here will cost me an hour daily. > By the same token of having a different bar for the Jenkins presubmit vs. what's run locally, I think it makes a lot of sense to run a different command for iterative development than you run before creating a pull request. E.g. during development I'll often run only one test rather than the entire suite, but do run the entire suite occasionally (often before commit, especially before pushing). The contributors guild should give a suggested command to run before creating a PR, right in the docs of how to create a PR, which may be more expensive than what you run during development. IMHO, this should be fairly comprehensive (certainly tests and checkstyle, maybe javadoc and findbugs). This should be the "default" command that the one-time-contributor should know. For those compiling 50x or more a day, I think the burden of learning a second (or more) cheaper commands is not high, and we could even put such a thing in the docs (and hopefully a common maven convention like "mvn test"). I've listed the fraction of commits I think will break one of the following > if that property is not tested: > > * compiling (100%) > * tests (100%) > * checkstyle (90%) > * javadoc (30%) > * findbugs (5%) > * rat (1%) > > So you can see where I stand and why. I'm sorry that 1/20 PRs has Apache > RAT catch a licensing issue or Findbugs catch a threading issue -- you can > always get a larger set of the precommit checks using -Prelease, though of > course the integration tests and runnableonservice tests may catch more > issues still. But I want my developer minutes back for the 95%+ of the > rest. > > Dan >
