@Kirk, I totally understand the pain that you speak of. I too agree that
every line of changed code should have a test confirming that behavior
was not changed.
I don't believe that we need to go as far as revoking committer rights
and reviewing each committer again, BUT it would be AMAZING that out of
our 100 committers, 80% of them would be more active in PR reviews,
mailing lists and in the end active on the project outside of their
focus area.
I do want to remind all Geode committers, it IS your responsibility to
be part of the PR review cycle. I will hold myself just as accountable
to this than what I hold every committer to, as I've been just as lazy
as the rest of them.
BUT
The reality is:
1. Geode code is HUGELY complex and NOT a test complete as we'd like
2. In the past single small changes have caused failures the were
completely unforeseen by anyone
3. In the past commits with single reviewers, have caused backward
compatibility issues which were only caught by chance in unrelated
testing.
4. There are 100 committers on Geode, and we keep on arguing that it is
hard to get PR's reviewed and that is why it is ok to have only 1
reviewer per PR.
5. There seems to be majority (unstated) opinion of: "why change, it
has been working for us so far." (I call is unstated, because being
silent means you agree with the status quo)
6. With requiring only 1 reviewer on code submissions, we are possibly
creating areas of the code only understood by a few.
IF, we as a project, have decided that all code shall enter only through
the flow of PR, then why not extend the QA cycle a little by requiring
more eyes. Lazy consensus, is as stated, lazy and would only work in a
project where the levels of complexity and size are not as high as
Geode's. In addition, with PR submissions, we have admitted that we are
human and could make mistakes and in an already complex environment and
to the best of our ability get it wrong.
Now, there are commits that really do not require 3 pairs of eyes,
because spelling mistakes and typos don't need consensus. But any time
code logic was amended, this needs to be reviewed.
I have seen different approach to code submissions:
* The submitter of the PR is NOT the committer of the PR. This task is
the responsibility of another committer(s) to review, approve and
finally merge in.
* Smaller amount of committers with higher numbers of contributors.
Yes, this does create a bottleneck, but it promotes a sense of pride
and responsibility that individual feels. Possibly a greater
understanding of the target module is promoted through this approach
as well.
Now, I don't say we as a project should follow these strict or
restricting approaches, but from my perspective, if we as a project
argue that we struggle to find 3 reviewers out of 100, then there are
bigger problems in the project than we anticipated. It is not a lack of
trust in our committers, to me it is a sense of pride that I want other
committers to confirm that I've delivered code to the high standard that
we want to be known for. Whilst it is painful to go through the process,
but if done correctly it is beneficial to all involved, as differing
opinions and approaches can be shared and all can learn from.
In addition, I have personally stumbled upon a few PR's, which upon
review found to be lacking in the areas of best practices of code and/or
design.
I fully support the notion of 3 reviewers per PR. I'm also going to take
it one step further, in the list of reviewers, there is at least 1
reviewer that is not part of a team, as this might drive a unbiased view
of the code and approach. I would also like to encourage ALL committers
to review code outside of the focus area. This will only promote a
broader understanding of the project codebase. I also support the notion
of a pair/mobbing reviews, if a reviewer does not understand the problem
area enough to effectively review, OR where the solution is not apparent.
--Udo
On 6/4/19 16:49, Kirk Lund wrote:
I'm -1 for requiring N reviews before merging a commit.
Overall, I support Lazy Consensus. If I post a PR that fixes the
flakiness
in a test, the precheckin jobs prove it, and it sits there for 2 weeks
without reviews, then I favor merging it in at that point without any
reviews. I'm not going to chase people around or spam the dev list over
and
over asking for reviews. Nothing in the Apache Way says you have to do
reviews before committing -- some projects prefer "commit then review"
instead of "review then commit". You can always look at the code someone
changed and you can always change it further or revert it.
I think if we don't trust our committers then we have a bigger systemic
problem that becoming more strict about PR reviews isn not going to fix.
Overall, I also favor pairing/mobbing over reviews. Without being there
during the work, a reviewer lacks the context to understand why it was
done
the way it was done.
If we cannot establish or maintain trust in committers, then I think we
should remove committer status from everyone and start over as a project,
proposing and accepting one committer at a time.
Instead of constraints on reviews, I would prefer to establish new
criteria
for coding such as:
1) all classes touched in a PR must have a unit test created if none
exists
2) all code touched in a PR must have unit test coverage (and possibly
integration test coverage) specific to the changes
3) all new classes must have full unit test coverage
4) all code touched in a PR must follow clean code principles (which
would
obviously need defining on the wiki)
Then it becomes the responsibility of the author(s) and committer(s) of
that PR to ensure that the code and the PR follows the project's criteria
for code quality and test coverage. It also becomes easier to measure the
PRs of a non-committer to determine if we think they would make a good
committer (for example, do they adhere to clean code quality and unit
testing with mocks? -- along with any other criteria).
On Thu, May 30, 2019 at 3:51 PM Owen Nichols <onich...@pivotal.io>
wrote:
It seems common for Geode PRs to get merged with only a single green
checkmark in GitHub.
According to https://www.apache.org/foundation/voting.html we should
not
be merging PRs with fewer than 3 green checkmarks.
Consensus is a fundamental value in doing things The Apache Way. A
single
+1 is not consensus. Since we’re currently discussing what it takes to
become a committer and what standards a committer is expected to
uphold, it
seems like a good time to review this policy.
GitHub can be configured to require N reviews before a commit can be
merged. Should we enable this feature?
-Owen
VOTES ON CODE MODIFICATION <
https://www.apache.org/foundation/voting.html#votes-on-code-modification>
For code-modification votes, +1 votes are in favour of the proposal, but
-1 votes are vetos <https://www.apache.org/foundation/voting.html#Veto>
and kill the proposal dead until all vetoers withdraw their -1 votes.
Unless a vote has been declared as using lazy consensus <
https://www.apache.org/foundation/voting.html#LazyConsensus> , three +1
votes are required for a code-modification proposal to pass.
Whole numbers are recommended for this type of vote, as the opinion
being
expressed is Boolean: 'I approve/do not approve of this change.'
CONSENSUS GAUGING THROUGH SILENCE <
https://www.apache.org/foundation/voting.html#LazyConsensus>
An alternative to voting that is sometimes used to measure the
acceptability of something is the concept of lazy consensus <
https://www.apache.org/foundation/glossary.html#LazyConsensus>.
Lazy consensus is simply an announcement of 'silence gives assent.’ When
someone wants to determine the sense of the community this way, it
might do
so with a mail message such as:
"The patch below fixes GEODE-12345; if no-one objects within three days,
I'll assume lazy consensus and commit it."
Lazy consensus cannot be used on projects that enforce a
review-then-commit <
https://www.apache.org/foundation/glossary.html#ReviewThenCommit>
policy.
