> > If we trust committers, why review at all? Just commit... and we might > catch a problem, we might not.
Honestly that to me would be the ideal state. However, our test coverage and code quality is nowhere near to allow for that. What I was referring to is different though. I didn't say "trust them to write perfect code", but trust " to decide how much review they require to feel comfortable". In some cases this might mean one review and in others maybe two, three or even more and maybe even by very specific people. On Wed, Jun 5, 2019 at 11:31 AM Udo Kohlmeyer <u...@apache.org> wrote: > Alexander, thank you for your response. And yes, change is uncomfortable > and in some cases more reviewers would not have caught issues. BUT, more > people would have seen the code, maybe become more familiar with it, etc... > > I don't say don't trust committers, as I am one. But I also know that I > mistakes are made regardless of intent. If we trust committers, why > review at all? Just commit... and we might catch a problem, we might not. > > --Udo > > On 6/5/19 11:20, Alexander Murmann wrote: > > Udo, I agree with many of the pains you are addressing, but am > pessimistic > > that having more reviewers will solve them. > > > > You are absolutely correct in calling out that the code is ugly complex > and > > missing coverage. The best way to address this is to clean up the code > and > > improve coverage. You say yourself "In the past single small changes have > > caused failures the were completely unforeseen by anyone". I don't think > > more eyeballs will go a long way in making someone see complex bugs > > introduced by seemingly safe changes. > > > > I also am concerned that introducing a hurdle like this will make > > committers more excited to review PRs with care, but rather might lead to > > less care. It would be great of our committers were more passionate > about > > PR reviews and do them more often, but forcing it rarely accomplishes > that > > goal. > > > > I'd rather see us trust our committers to decide how much review they > > require to feel comfortable about their work and use the time saved to > > address the root of the problem (accidental complexity & lack of test > > coverage) > > > > On Wed, Jun 5, 2019 at 11:03 AM Udo Kohlmeyer <u...@apache.org> wrote: > > > >> @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. > >>>> > >>>> >
