+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 > >
