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
