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
