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