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