spark-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Patrick Wendell <>
Subject Semantics of LGTM
Date Sun, 18 Jan 2015 01:40:39 GMT
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

"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
- "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:
For additional commands, e-mail:

View raw message