The wiki does not seem to be operational ATM, but I will do this when it is back up.
On Mon, Jan 19, 2015 at 12:00 PM, Patrick Wendell <pwend...@gmail.com> wrote: > Okay - so given all this I was going to put the following on the wiki > tentatively: > > ## Reviewing Code > Community code review is Spark's fundamental quality assurance > process. When reviewing a patch, your goal should be to help > streamline the committing process by giving committers confidence this > patch has been verified by an additional party. It's encouraged to > (politely) submit technical feedback to the author to identify areas > for improvement or potential bugs. > > If you feel a patch is ready for inclusion in Spark, indicate this to > committers with a comment: "I think this patch looks good". Spark uses > the LGTM convention for indicating the highest level of technical > sign-off on a patch: simply comment with the word "LGTM". An LGTM is a > strong statement, it should be interpreted as the following: "I've > looked at this thoroughly and take as much ownership as if I wrote the > patch myself". If you comment LGTM you will be expected to help with > bugs or follow-up issues on the patch. Judicious use of LGTM's is a > great way to gain credibility as a reviewer with the broader > community. > > It's also welcome for reviewers to argue against the inclusion of a > feature or patch. Simply indicate this in the comments. > > - Patrick > > On Mon, Jan 19, 2015 at 2:40 AM, Prashant Sharma <scrapco...@gmail.com> wrote: >> 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 >>> > > >>> > > >>> > >> >> --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@spark.apache.org For additional commands, e-mail: dev-h...@spark.apache.org
