Okay - so given all this I was going to put the following on the wiki tentatively:
## Reviewing Code Community code review is Spark's fundamental quality assurance process. When reviewing a patch, your goal should be to help streamline the committing process by giving committers confidence this patch has been verified by an additional party. It's encouraged to (politely) submit technical feedback to the author to identify areas for improvement or potential bugs. If you feel a patch is ready for inclusion in Spark, indicate this to committers with a comment: "I think this patch looks good". Spark uses the LGTM convention for indicating the highest level of technical sign-off on a patch: simply comment with the word "LGTM". An LGTM is a strong statement, it should be interpreted as the following: "I've looked at this thoroughly and take as much ownership as if I wrote the patch myself". If you comment LGTM you will be expected to help with bugs or follow-up issues on the patch. Judicious use of LGTM's is a great way to gain credibility as a reviewer with the broader community. It's also welcome for reviewers to argue against the inclusion of a feature or patch. Simply indicate this in the comments. - Patrick On Mon, Jan 19, 2015 at 2:40 AM, Prashant Sharma <scrapco...@gmail.com> wrote: > Patrick's original proposal LGTM :). However until now, I have been in the > impression of LGTM with special emphasis on TM part. That said, I will be > okay/happy(or Responsible ) for the patch, if it goes in. > > Prashant Sharma > > > > On Sun, Jan 18, 2015 at 2:33 PM, Reynold Xin <r...@databricks.com> wrote: >> >> Maybe just to avoid LGTM as a single token when it is not actually >> according to Patrick's definition, but anybody can still leave comments >> like: >> >> "The direction of the PR looks good to me." or "+1 on the direction" >> >> "The build part looks good to me" >> >> ... >> >> >> On Sat, Jan 17, 2015 at 8:49 PM, Kay Ousterhout <k...@eecs.berkeley.edu> >> wrote: >> >> > +1 to Patrick's proposal of strong LGTM semantics. On past projects, >> > I've >> > heard the semantics of "LGTM" expressed as "I've looked at this >> > thoroughly >> > and take as much ownership as if I wrote the patch myself". My >> > understanding is that this is the level of review we expect for all >> > patches >> > that ultimately go into Spark, so it's important to have a way to >> > concisely >> > describe when this has been done. >> > >> > Aaron / Sandy, when have you found the weaker LGTM to be useful? In the >> > cases I've seen, if someone else says "I looked at this very quickly and >> > didn't see any glaring problems", it doesn't add any value for >> > subsequent >> > reviewers (someone still needs to take a thorough look). >> > >> > -Kay >> > >> > On Sat, Jan 17, 2015 at 8:04 PM, <sandy.r...@cloudera.com> wrote: >> > >> > > Yeah, the ASF +1 has become partly overloaded to mean both "I would >> > > like >> > > to see this feature" and "this patch should be committed", although, >> > > at >> > > least in Hadoop, using +1 on JIRA (as opposed to, say, in a release >> > > vote) >> > > should unambiguously mean the latter unless qualified in some other >> > > way. >> > > >> > > I don't have any opinion on the specific characters, but I agree with >> > > Aaron that it would be nice to have some sort of abbreviation for both >> > the >> > > strong and weak forms of approval. >> > > >> > > -Sandy >> > > >> > > > On Jan 17, 2015, at 7:25 PM, Patrick Wendell <pwend...@gmail.com> >> > wrote: >> > > > >> > > > I think the ASF +1 is *slightly* different than Google's LGTM, >> > > > because >> > > > it might convey wanting the patch/feature to be merged but not >> > > > necessarily saying you did a thorough review and stand behind it's >> > > > technical contents. For instance, I've seen people pile on +1's to >> > > > try >> > > > and indicate support for a feature or patch in some projects, even >> > > > though they didn't do a thorough technical review. This +1 is >> > > > definitely a useful mechanism. >> > > > >> > > > There is definitely much overlap though in the meaning, though, and >> > > > it's largely because Spark had it's own culture around reviews >> > > > before >> > > > it was donated to the ASF, so there is a mix of two styles. >> > > > >> > > > Nonetheless, I'd prefer to stick with the stronger LGTM semantics I >> > > > proposed originally (unlike the one Sandy proposed, e.g.). This is >> > > > what I've seen every project using the LGTM convention do (Google, >> > > > and >> > > > some open source projects such as Impala) to indicate technical >> > > > sign-off. >> > > > >> > > > - Patrick >> > > > >> > > >> On Sat, Jan 17, 2015 at 7:09 PM, Aaron Davidson >> > > >> <ilike...@gmail.com> >> > > wrote: >> > > >> I think I've seen something like +2 = "strong LGTM" and +1 = "weak >> > LGTM; >> > > >> someone else should review" before. It's nice to have a shortcut >> > > >> which >> > > isn't >> > > >> a sentence when talking about weaker forms of LGTM. >> > > >> >> > > >> On Sat, Jan 17, 2015 at 6:59 PM, <sandy.r...@cloudera.com> wrote: >> > > >>> >> > > >>> I think clarifying these semantics is definitely worthwhile. Maybe >> > this >> > > >>> complicates the process with additional terminology, but the way >> > > >>> I've >> > > used >> > > >>> these has been: >> > > >>> >> > > >>> +1 - I think this is safe to merge and, barring objections from >> > others, >> > > >>> would merge it immediately. >> > > >>> >> > > >>> LGTM - I have no concerns about this patch, but I don't >> > > >>> necessarily >> > > feel >> > > >>> qualified to make a final call about it. The TM part acknowledges >> > the >> > > >>> judgment as a little more subjective. >> > > >>> >> > > >>> I think having some concise way to express both of these is >> > > >>> useful. >> > > >>> >> > > >>> -Sandy >> > > >>> >> > > >>>> On Jan 17, 2015, at 5:40 PM, Patrick Wendell <pwend...@gmail.com> >> > > wrote: >> > > >>>> >> > > >>>> Hey All, >> > > >>>> >> > > >>>> Just wanted to ping about a minor issue - but one that ends up >> > having >> > > >>>> consequence given Spark's volume of reviews and commits. As much >> > > >>>> as >> > > >>>> possible, I think that we should try and gear towards "Google >> > > >>>> Style" >> > > >>>> LGTM on reviews. What I mean by this is that LGTM has the >> > > >>>> following >> > > >>>> semantics: >> > > >>>> >> > > >>>> "I know this code well, or I've looked at it close enough to feel >> > > >>>> confident it should be merged. If there are issues/bugs with this >> > code >> > > >>>> later on, I feel confident I can help with them." >> > > >>>> >> > > >>>> Here is an alternative semantic: >> > > >>>> >> > > >>>> "Based on what I know about this part of the code, I don't see >> > > >>>> any >> > > >>>> show-stopper problems with this patch". >> > > >>>> >> > > >>>> The issue with the latter is that it ultimately erodes the >> > > >>>> significance of LGTM, since subsequent reviewers need to reason >> > about >> > > >>>> what the person meant by saying LGTM. In contrast, having strong >> > > >>>> semantics around LGTM can help streamline reviews a lot, >> > > >>>> especially >> > as >> > > >>>> reviewers get more experienced and gain trust from the >> > > >>>> comittership. >> > > >>>> >> > > >>>> There are several easy ways to give a more limited endorsement of >> > > >>>> a >> > > >>>> patch: >> > > >>>> - "I'm not familiar with this code, but style, etc look good" >> > (general >> > > >>>> endorsement) >> > > >>>> - "The build changes in this code LGTM, but I haven't reviewed >> > > >>>> the >> > > >>>> rest" (limited LGTM) >> > > >>>> >> > > >>>> If people are okay with this, I might add a short note on the >> > > >>>> wiki. >> > > >>>> I'm sending this e-mail first, though, to see whether anyone >> > > >>>> wants >> > to >> > > >>>> express agreement or disagreement with this approach. >> > > >>>> >> > > >>>> - Patrick >> > > >>>> >> > > >>>> >> > --------------------------------------------------------------------- >> > > >>>> To unsubscribe, e-mail: dev-unsubscr...@spark.apache.org >> > > >>>> For additional commands, e-mail: dev-h...@spark.apache.org >> > > >>> >> > > >>> >> > > >>> --------------------------------------------------------------------- >> > > >>> To unsubscribe, e-mail: dev-unsubscr...@spark.apache.org >> > > >>> For additional commands, e-mail: dev-h...@spark.apache.org >> > > >> >> > > >> > > --------------------------------------------------------------------- >> > > To unsubscribe, e-mail: dev-unsubscr...@spark.apache.org >> > > For additional commands, e-mail: dev-h...@spark.apache.org >> > > >> > > >> > > > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@spark.apache.org For additional commands, e-mail: dev-h...@spark.apache.org
