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
