That looks good, thanks Stephen. --Ted
On Thu, Jul 12, 2018 at 5:23 PM Stephen Mallette <[email protected]> wrote: > I'm going to quickly summarize this thread so that hopefully by Monday we > have a plan to follow. > > We seem to agree that in the future we will go with the following review > -the-commit (RTC) process: > > 1. Each change to TinkerPop release branches requires 3 +1s from committers > and no -1s OR > 2. A single +1 from a committer and a 1 week review period for objections > (i.e. "cool down period") at which point we will assume a lazy consensus > 3. The single +1 may come from the committer who submitted the PR in the > first place > 4. Committers are trusted with their changes but are expected to request > reviews for complex changes as necessary and not rely strictly on lazy > consensus > 5. commit-the-review (CTR) procedures remain unchanged > > If there are no final comments, I will adjust our dev documentation > accordingly (and then merge a bunch of PRs!) > > Thanks for everyone's participation, > > Stephen > > On Wed, Jul 11, 2018 at 6:50 AM Stephen Mallette <[email protected]> > wrote: > > > oops - Pieter, i read your post last night then replied this morning > > thinking I remembered everything you wrote - you actually called for > > different email/jira lists as well.....I guess that would be helpful to > > some but not others. For me personally, that would be massively > burdensome > > tbh. > > > > On Wed, Jul 11, 2018 at 6:46 AM Stephen Mallette <[email protected]> > > wrote: > > > >> Thanks for everyone's thoughts - some replies, first to Jason: > >> > >> > but I agree that nagging is not a great path forward. > >> > >> Robert Dale has already expressed his sadness that my nags are going > away > >> > >> > It'd be great to have these examples added to the maintainer > >> guidelines. > >> > >> i've said as much before in different places but it's not in the dev > >> docs. of course, depending on how this thread ends the dev docs will > need > >> some changes in this area all around so we'll see about adding such > things. > >> > >> > 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. > >> > >> So, here's my thinking on "major feature" - if the committer had done > >> things properly the "major feature" would have already had major > >> visibility. There would have been some form of DISCUSS thread ahead of > >> time, plus a JIRA ticket with more information. If it was a "major" > third > >> party submission, a committer would not +1 and walk away but suggest to > the > >> third-party that we need to bring this to the attention of dev in some > way. > >> How about this, perhaps in the DISCUSS/JiRA anyone might call for a > "review > >> of 3" (reserving of course for a -1 veto) in which case the "cooling > >> period" wouldn't apply and we'd need the 3 +1s from committers. > >> > >> As for Pieter: > >> > >> > Perhaps for version 4 the project should be broken up > >> > >> Maybe, though I've come to to really like our giant uber project. Having > >> everything together has so many great benefits and somehow we've > managed to > >> keep the whole thing relatively easy to build and deploy. Breaking up > the > >> project repositories however would not allow you to really hide from the > >> expansiveness we've grown in to. This dev list would still be the > location > >> for all discussion and we'd still likely have a single JIRA instance so > I > >> don't see that providing much relief if you're feeling that way > >> unfortunately. > >> > >> > >> > >> On Tue, Jul 10, 2018 at 4:19 PM pieter gmail <[email protected]> > >> wrote: > >> > >>> Hi, > >>> > >>> I feel like the project has become a bit too big and dispersed. A large > >>> portion of the emails, jira or otherwise are irrelevant to my > >>> interest/time/work. > >>> > >>> Perhaps for version 4, TinkerPop could be broken up into more focused > >>> projects with their own jira/email/process management. > >>> > >>> gremlin-language > >>> gremlin-server > >>> js-driver > >>> python-driver > >>> java-driver > >>> .net-driver > >>> reference implementation > >>> ... > >>> > >>> Thanks > >>> Pieter > >>> > >>> > >>> > >>> > >>> Perhaps for version 4 the project should be broken up > >>> > >>> On 10/07/2018 22:01, Jason Plurad wrote: > >>> > 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. > >>> >>>>>>> > >>> > >>> >
