Thank you Dan,
Some guidance around how we can go about reviewing the code.
I want to remind ALL
committers..https://www.apache.org/dev/new-committers-guide.html#committer-responsibilities
It states "/Each committer has a responsibility to monitor the changes
made for potential issues, both coding and legal."/
I cannot see a reason to have any reviewers on trivial (spelling, typos).
Post that, everything should have at least a view reviewers. 1 does not
an opinion (or reviewed code) make, and I must agree with the statement
that Owen made. 3 reviewers.
Yes it is a real pita. As it takes dedication from ppl to review code
and it takes away from our daily lives and the limited time we have to
dedicate to it. Yet, I cannot understand how out of 70 committers (I'm
using 70% fudge factor) we cannot get 3 reviews on correctness, style,
potential bugs. In addition committers CAN nominate reviewers on PR's.
In addition to the nomination of reviewers, I would advocate that
reviewers of code not be more than 66% of a potentially biased team
members. (given that I know many committers are employed by the same
company).
@Naba I agree, there is more work now. I now as a committer actually
have to review the code of a fellow committer. BUT we all know that the
space and code we work on is complex. Concurrent, distributed systems
are not easy and being too close to the problem could cause blinders to
issues which someone more removed could spot. The opposite is also true,
too removed will only evaluate on basic criteria and might not spot
issues. Regardless of this, we have many troops that can do work.. and 1
code review 1-2 twice a week is not going to kill us.
I would also like to request of everyone, that when submitting a PR,
keep an eye on it. Track it, ping the @dev channel if no progress is
made. Oversights can happen and in some cases emails can be filtered out
with new PR's or comments made on one's own PR.
--Udo
On 5/31/19 11:33, Dan Smith wrote:
As others pointed out - I think it's really important to remember that
people and community are much more important than process. That is a key
part of "The Apache Way" - it's not a set very specific rules, but more of
a philosophy of building an open community. I think this page has a good
take on the apache way - http://theapacheway.com/.
The page I linked also mentions "Getting good enough consensus is often
better than voting." Apache is about consensus building, *not* about
voting. Ideally, the only things we should formally vote on are
irreversible decisions such as a release or new committer.
Regarding code reviews, I think having a really strict process implies that
we don't trust our committers. Rather than that, maybe have some guidelines
for committers who aren't sure how much of a review to get. Here's what I
feel like I've been trying to follow:
1. Fixing a typo, broken spotless, etc. - just push it!
2. Straightforward change - get at least one reviewer. Maybe a second
author on the commit should count here?
3. Technically challenging, complicated change - get multiple reviewers
4. New Public API, large features - Write up a wiki page and have a
discussion on the dev list to get feedback
For all of these, if someone rejects your PR/feature, fix the issues or
talk with them. A good rule of thumb is if you are frustrated or annoyed by
what they said - talk to them one-on-one first instead of answering
directly in the PR/thread. If you a really pissed, wait a day and then talk
to them one-on-one.
-Dan
On Fri, May 31, 2019 at 11:00 AM Helena Bales <hba...@pivotal.io> wrote:
Thanks for the filter, Jinmei!
+1 to Naba and Bruce.
I will add that I think we should have a formal policy of getting *a*
review (and more for large changes), and that PRs that are merged without
one should include (in the PR) a comment providing a justification for this
merge, so that committers can review the reasoning later on if needed. This
has the benefits of being low impact on our current workflow, but also
increasing transparency, accountability, and thoughtfulness around the
proposed changes and their impact.
On Fri, May 31, 2019 at 10:42 AM Jinmei Liao <jil...@pivotal.io> wrote:
I was told that screenshot that I sent earlier got filtered out by the
dev
list. Basically, the filter puts "notificati...@github.com" in the
"From"
section, and put "review_reques...@noreply.github.com" in the "Doesn't
have" section of the form.
On Fri, May 31, 2019 at 10:36 AM Anthony Baker <aba...@pivotal.io>
wrote:
On May 31, 2019, at 10:01 AM, Owen Nichols <onich...@pivotal.io>
wrote:
We chose to make Geode an Apache open source project for a reason.
If
we no longer wish to embrace The Apache Way <
https://www.apache.org/theapacheway/index.html>, perhaps we should
reconsider.
I strongly disagree with the assertion that we are not following the
Apache Way because we aren’t doing RTC. Please take a look around
other
ASF communities and compare that to our approach. I think you’ll see a
lot
of similarities in the way we review GitHub pull requests.
If we believe that reviewing each other’s code changes is an onerous
burden of no value, we should question why. The long-term success of
Geode depends on sharing of knowledge, not “cowboy coders”. 3 reviews
means now 3 other people are more familiar with that part of the code…
Yes of course: community >> code. Can you point me to cases of
“cowboy
coding” in Geode? I’m not seeing it but happy to be convinced
otherwise.
If apathy is our thing, Apache does allows for “lazy consensus”, but
you
have to declare that you will be using it, e.g. “This PR fixes
GEODE-123456; if no-one objects within three days, I'll assume lazy
consensus and merge it.”
IMO lazy consensus does not imply apathy.
Anthony
--
Cheers
Jinmei
