On Fri, Feb 10, 2017 at 11:54 AM, Davor Bonaci <da...@google.com.invalid> wrote:
> I think Dan's framework of thinking is right -- what is the probability of > something finding a real issue, vs. the cost of running that all the time. > > Obviously, we cannot run *everything* all the time. There's an infinite > number of things to run and infinite matrix of configurations. Many tests > are not even possible to execute locally like testing scale, performance, > compatibility across platforms, etc. So, we run locally *the least*, > slightly more in presubmit, slightly more in post-submit, even more > nightly, and as much as possible for each release. Of course, it is great > to catch issues as soon as possible, but the cost is prohibitive in terms > of time and productivity. > > This may sound a bit tangential so far... but, I think Aviem's (and others) > point of view come from a social aspect. As a contributor, nobody wants to > be "embarrassed" by proposing a pull request that is broken and has obvious > problems. One might think of themselves as failing to do a good job in this > particular case. So, instead, I might want to default to running more > things locally to prevent the "embarrassment". > +1 It's also disheartening to think a PR is ready for folks to look at, only to notice (later, when you've moved on to doing something else and a Jenkins issue comes in) that you have, say, some trivial checkstyle whitespace issue. Also, in my experience, newcomers usually value direction and the ability to algorithmically, privately verify their code quite a bit. On the other hand, having to start up a spark cluster or create GCP credentials is a higher burden that is often better offloaded to something like Jenkins. Looking more broadly, however, there's a huge difference in things that the > default local execution *and* pre-commit don't exercise. It is really huge > and many breakages are caught in post commit, nightlies, or, at the very > end, cherrypicked into the release. So, I'd like to argue that nobody > should ever be embarrassed by proposing a pull request. It is normal, > expected, reasonable that Jenkins will catch issues -- that's why we have > it. Instead, think in terms of "error budget" -- increase individual > productivity by sacrificing occasional breakages. It is also completely > normal that a PR causes a post-commit breakage at times, or that a release > is delayed because of some PR -- this is all normal and fits into your > "error budget". > <aside>In my opinion, post-commits should catch virtually nothing, or at least nothing deterministic. Once we have a mergebot, there's no reason we should't test virtually everything we do on those runs as a last step before every merge instead/as well.</aside>. One easy improvement that I know Jason is looking at is to separate the > precommit signal into multiple signals. So, instead of a one "red" signal, > contributors can get five "greens" and one "red", which may help decrease > this social impact. > That would be great. > Hopefully, this is a convincing argument to use this framework in deciding > this matter. On the lower-lever issue, I think rat and findbugs have a low > probability of finding an issue in most cases. > > (Also, an explicit +1 to Kenn's point of view of getting people to the PR > so we can work with them, as opposed to blocking them locally before they > interact with us.) > We should also factor in how (naively) actionable the errors from the various tools are. Linters are typically pretty explicit about what needs to be changed, other tools sometimes less so. > On Fri, Feb 10, 2017 at 11:50 AM, Kenneth Knowles <k...@google.com.invalid> > wrote: > > > Since the discussion has returned to the thread rather than Dan's PR, I > > want to paraphrase the point I feel strongest about here: > > > > *For a new contributor, I want to minimize the distance between them > > deciding to hack and becoming our friends.* > > > > So I don't want them to have to learn much, if anything, about our idioms > > prior to opening a PR. That includes checkstyle, findbugs, javadoc, RAT, > > (any others?). Once a PR is open, we can welcome them to the community, > > help them understand any failure that happened via Jenkins, tell them the > > command to run the more thorough test, or even issue little fix PRs > against > > their branch. If they get blocked by nitpicky or confusing failures in > > their console or while hacking, they might (reasonably, IMO) decide to go > > do something else. > > > > Folks other than newcomers can learn a repertoire of commands, like > Robert > > says. So we shouldn't consider them (aka "us") so much when deciding > > whether "fast" or "slow" is the default, as long as we can explicitly > > choose. > > > > Kenn > > > > On Fri, Feb 10, 2017 at 11:00 AM, Robert Bradshaw < > > rober...@google.com.invalid> wrote: > > > > > > 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"). > > > > > > > > Kenn > > > > > > > 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 > > > > > > > > > >
