Thanks for starting this conversation, Stephen. Lots of interesting tidbits here, and perhaps some we can apply to other OSS projects.
> I'm not sure if committers/PMC members have just not had time to do reviews or have not felt comfortable doing them Probably a combination of both, especially with the GLVs. > I personally chase votes in the background to get PRs to merge.....and, I don't want to do that anymore. Amazing that you did that, but I agree that nagging is not a great path forward. > it is perfectly fine to review/VOTE in the following manner (as examples) It'd be great to have these examples added to the maintainer guidelines. When I do code reviews, sometimes I feel like one-liner votes are a bit of a cop out, but having examples like this would lower the mental hurdle to getting started on reviewing. > It would also be nice for non-committers to do reviews - i don't know how to encourage that behavior though. I agree on this, and it would be particularly important on areas of the code where we only have one primary committer, like each GLV. If we come to agreement on a new policy, I'd suggest that if we get the docs written up and published, then we can mention it on gremlin-users sort of as a heads up to those interested in getting more involved. Their participation helps drive out releases, and new releases attract more users. Regarding the proposal, a single binding +1 from a committer with a 1 week lazy consensus sounds fine to me. If the contribution is a major feature or significant change, the expectation is that the committer realizes this and holds it open for 3 votes before committing. On Tue, Jul 10, 2018 at 1:46 PM, Stephen Mallette <[email protected]> wrote: > Good point, Ted - that wasn't clear and in truth I didn't think that > through well. I think we could say that that the +1 would come from a > committer. If the committer and submitter are one in the same then it has > its single VOTE and technically, the PR just goes through the week long > cooling period and could be merged after that. In the event the PR > submitter is not a committer then they would require at least one committer > to be on board with a +1 and a week long wait. > > Ideally, I think we can trust committers enough to do smart things with > this change in process. I would hope that a committer who submits a PR that > is especially complex and touches scary code parts or breaks user/provider > APIs requests an actual review from another committer who might be able to > spot some problems even if the 1 week cool down passes by. I don't want to > subvert the good things that reviews do, but I don't want them holding up > simple code changes either. I'd really like it if we introduced this change > and we still got multiple +1s on PRs. It would also be nice for > non-committers to do reviews - i don't know how to encourage that behavior > though. > > > > > On Tue, Jul 10, 2018 at 1:26 PM Ted Wilmes <[email protected]> wrote: > > > I fell way off the PR review train, I'll get back on. For clarification, > is > > that a +1 on top of the submitter +1? I'm thinking you > > all just meant the submitter's +1 would be adequate after the lazy > > consensus period but wanted to be sure. I'd be fine to moving with that. > My > > impression is that with the folks involved, if a submitter feels that > > another set of eyes is really required and lazy consensus is not > adequate, > > regardless of the policy, that review will be sought and performed prior > to > > merge. > > > > --Ted > > > > On Tue, Jul 10, 2018 at 11:44 AM Stephen Mallette <[email protected]> > > wrote: > > > > > > It looks like its disabled for this project. > > > > > > I don't think we can use the GitHub integration without getting off our > > > Apache Mirror (which we've discussed, but not really pulled the trigger > > on > > > for no particular reason other than the hassle of changing everything). > > > > > > > Does it have to be in that order? > > > > > > I was thinking that the as long as there is a single +1 at any time in > > that > > > week (or after that week) then it would be good to merge > > > > > > On Tue, Jul 10, 2018 at 12:36 PM Robert Dale <[email protected]> > wrote: > > > > > > > There might be a better alternative to privately nagging ;-) Github > > has > > > a > > > > feature on the sidebar that can be used to request reviews from > > > individuals > > > > or groups. The heading has 'Reviewers' and, when it's active, has a > > gear > > > > icon to select people. Github will then email the reviewers with the > > > > request. It looks like its disabled for this project. > > > > > > > > I like the idea of adding the option of having a single vote and a > week > > > to > > > > soak. Does it have to be in that order? Or can the vote take place > > any > > > > time during that week? Otherwise, you could end up with no one > voting > > in > > > > the first week, get a vote, then waiting another week. > > > > > > > > Robert Dale > > > > > > > > > > > > On Tue, Jul 10, 2018 at 7:22 AM Jorge Bay Gondra < > > > [email protected] > > > > > > > > > wrote: > > > > > > > > > I'm +1 on the idea of switching to lazy consensus after a single > > > binding > > > > > plus one and a week for objection. TinkerPop has so many different > > > > modules > > > > > / areas and committers have different expertise that is hard to > get 3 > > > > votes > > > > > on something. > > > > > > > > > > Other projects have the concept of main "reviewer" and this would > be > > > very > > > > > similar, a committer that was responsible for reviewing the code > and > > to > > > > > check that everything is in order. > > > > > > > > > > El mar., 10 jul. 2018 a las 13:01, Stephen Mallette (< > > > > [email protected] > > > > > >) > > > > > escribió: > > > > > > > > > > > I believe that the review process is not working so well anymore. > > I'm > > > > not > > > > > > sure if committers/PMC members have just not had time to do > reviews > > > or > > > > > have > > > > > > not felt comfortable doing them, but for the most part they > aren't > > > > > getting > > > > > > done and PRs are languishing. Personally, I like our process, but > > if > > > it > > > > > > takes 3+ weeks to deal with a PR like this: > > > > > > > > > > > > https://github.com/apache/tinkerpop/pull/879/files > > > > > > > > > > > > where all we did was remove deprecated methods, I don't think > we're > > > > ever > > > > > > going to get anything else through. As it stands, I personally > > chase > > > > > votes > > > > > > in the background to get PRs to merge.....and, I don't want to do > > > that > > > > > > anymore. > > > > > > > > > > > > I'll remind committers (and those interested in becoming > > committers) > > > > > that a > > > > > > "review" in our context doesn't have to be a full on review of > code > > > > where > > > > > > you hold this deep understanding of everything that is going on. > > That > > > > is > > > > > > awesome when that happens, but it is perfectly fine to > review/VOTE > > in > > > > the > > > > > > following manner (as examples): > > > > > > > > > > > > + VOTE +1 - ran docker integration tests and everything passes > > > > > > + VOTE +1 - reviewed the code in detail - solid pull request > > > > > > + VOTE +1 - agree with the principle of this pull request but > don't > > > > fully > > > > > > understand the code > > > > > > + VOTE +1 - read through the updated documentation and understand > > why > > > > > this > > > > > > is important, nice > > > > > > > > > > > > So basically, you can VOTE and just explain your position for why > > you > > > > > voted > > > > > > (or not explain). I would like to keep this process, however, if > we > > > > can't > > > > > > raise the VOTEs for whatever reason, then I'd like to suggest a > > > change. > > > > > > > > > > > > I'd suggest that we go to a slightly looser version of > > > > > review-then-commit, > > > > > > where we require the 3 binding +1 VOTEs as we have been doing OR > > we > > > > > > require a single binding +1 and 1 week for objection at which > point > > > we > > > > > have > > > > > > a form of lazy consensus. > > > > > > > > > > > > Honestly, I'd like to see some discussion on this from > > committers/PMC > > > > > > members and not go with the standard form of lazy consensus that > we > > > > > > typically end up with. However, if no one truly has anything to > > say, > > > > > > consider the 72 hours started now. > > > > > > > > > > > > > > > > > > > > >
