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
