I am a firm -1. I already have a python script that generates maintainers, but I would rather we look at the maintainer plugin which was contributed to gerrit by Cisco. https://gerrit.googlesource.com/plugins/maintainer/
As to Andrew's point, it is just as fair to propose that if a maintainer hasn't addressed a verified +1 in T+30, it is merged and it is then it will become a priority to the maintainer. About a month or so ago, Ole +2'd and merged a change of mine that has been sitting for 2 releases, because Andrew -1'd for doing some testing and then left it. Thank you, Ole. When I first became a comitter, I submitted a change that Danjan -1'd that the feedback was that it was agreed to not accept. Damjan +2'd and committed it on his own some 10 months later. Thank you, Damjan. There is a change in the system https://gerrit.fd.io/r/c/vpp/+/30559 that allows a user to build vpp if the system language is Chinese. The author tagged the maintainer, and hasn't gotten a response. I tested the patch and +1'd it to help save some time for the maintainer. A week or so later, another comitter was tagged. It is still sitting there. When the question of adding worker tests to the framework came up, Klement sent me a link to work he had done over a year earlier that was never merged. The Netgate folks had a changeset they were waiting a month or so for a review, then they were told that it was too close to the release to merge it. It was merged, but the argument that "because we held you back, we're going to hold you back some more" is very anti-community. I have MANY changes that I rely on that I cherry pick into my own builds. I had a change that added the names to the established unix sockets for troubleshooting. It was a good year old. Neale saw it a month or so ago and merged it. The maintainers did not. I have uncommitted changes right now that fix issues in the build system that are blocking other changes from being merged. Dave, Let me know how I can help. On Thu, Jan 28, 2021 at 11:36 AM Andrew Yourtchenko <ayour...@gmail.com> wrote: > Dave, > > On 28 Jan 2021, at 16:59, Dave Wallace <dwallac...@gmail.com> wrote: > > Andrew, > > The only issue I have with your proposal is that there is no way to > implement it. Gerrit auto-abandon doesn't have a way to detect 'T'. > > > We could approximate it by setting changeCleanup.abandonIfMergeable to > false, it’s not perfect, of course, but I think it will take care of most > of the true “dead” changes. > > It seems like we already do have gerrit do the indexing, so it’s no-cost > operation. > > > > While I agree with your idea that 'T' should be based on a time other than > the initial patch upload, I don't see a mechanism to do so. It would > appear to me that the only other solution would be to make the window > larger and anything up to the release cycle duration (120 days) seems > reasonable to me. > > The other point I would make is that auto-abandon is not deletion. Thus > it puts more ownership on the patch creator to poke the maintainer(s) for a > review. And I'm assuming that 'Restore' resets the clock on the > auto-abandon trigger. > > > True, and we could put the appropriate abandon message that would > describes what to do and explains why, so people don’t get upset/annoyed. > > > > Lastly, I have not heard a counter-proposal from either you or Ole on how > to clean up the current state of the queue. We're wasting cycles and > abusing the infra by letting the queue remain so large and it would be wise > to address that sooner rather than later. > > > If we decide to enable it, shout loudly on the list and give folks a month > warning before actually enable it, to touch the changes they intend on > working on. Then enable the plugin. Then start with whatever > nagging/reporting process from clean slate on top of the plugin being > already active. > > To be clear: I think that there are two separate things: > > 1) having a boatload of super outdated open changes > 2) avoiding such changes piling up in the future/improving the review time > > The enabling of the plugin proposal solves (1), which is an immediate > technical problem. > > Ole’s proposal seems to me to be aimed to address the (1) via (2), so my > comment was that I disagreed that the suggested approach of addressing (2) > is going to work, thus the suggested tweak. > > I still think it’s fine to enable the plugin (with not deleting the > mergeable changes config), and it will solve the (1), and then discuss how > to tackle (2). > > —a > > > Thanks, > -daw- > > On 1/28/2021 4:29 AM, Andrew 👽 Yourtchenko wrote: > > On 28 Jan 2021, at 10:10, Ole Troan <otr...@employees.org> > <otr...@employees.org> wrote: > > My impression is that there is a disconnect between someone putting > something on gerrit and a vpp maintainer reviewing and contributor merging. > > Absolutely agree on that. > > As a project we certainly can do better on managing the stream of changes. > With my release manager hat on, I have seen a non-trivial number of cases > where the patch sits for a while, then “oh damn it’s release time”, it gets > squeezed in last moment, with predictable consequences. > > > I was thinking of having a script processing the review queue and generating > reports for each maintainer. Then give each author a chance to get their code > reviewed and merged. > > if the maintainers don’t look at the new changes today, why would they look > at the reports on the changes they didn’t look at ? > > > I would like to try the gentle nudge first. If we go down the abandon route, > certainly not without sending alerts first. > > If a person is legit swamped, the robotic nags will just annoy them and will > go into recycle bin. I kinda like the idea of nudges but to have a chance to > help - they need to go to the mail list where the other people might jump > in... (of course the code owners have the final say but often there can be > basic things that can be done. Maybe even do it over the email altogether - > there was a recent experience on the list and I think it might not be a bad > idea to make it a more frequent occurrence... > > Then a possible sequence could be: > > T: author submits the patch without their own “-1/-2” or removed -1/-2 > T+7: the mail to vpp-dev is sent > T+30: autoabandon plugin triggers if no activity on the patch > > > —a > > > > So a tentative -1. > > Cheers > Ole > > > > > On 28 Jan 2021, at 09:19, Benoit Ganne (bganne) via lists.fd.io > <bganne=cisco....@lists.fd.io> <bganne=cisco....@lists.fd.io> wrote: > > +1 > > ben > > > -----Original Message----- > From: vpp-dev@lists.fd.io <vpp-dev@lists.fd.io> <vpp-dev@lists.fd.io> On > Behalf Of Dave Wallace > Sent: mercredi 27 janvier 2021 22:50 > To: vpp-dev@lists.fd.io > Subject: [vpp-dev] RFC: Enabling Gerrit Auto-Abandon job on VPP master > > Folks, > > There are currently 636 open Gerrit Reviews on VPP master [0], the oldest > being submitted on Jun 13, 2016 [1]! > > I would like to propose that the Gerrit Review Auto-Abandon job [2] to > reduce the size of the queue to a more reasonable length. Benefits include > (from [3]): > > ----- %< ----- > > Abandoning old inactive changes has the following advantages: > > it signals change authors that changes are considered outdated > > it keeps dashboards clean > > it reduces the load on the server (for open changes the mergeability > flag is recomputed whenever a change is merged) > > If a change is still wanted it can be restored by clicking on the Restore > button. > > ----- %< ----- > > I would like to propose the following configuration [2] for auto-abandon: > > changeCleanup.abandonAfter: 30d > changeCleanup.abandonIfMergeable: default (true) > changeCleanup.cleanupAccountPatchReview: default (false) > changeCleanup.abandonMessage: default > changeCleanup.startTime: Sat 00:00 > changeCleanup.interval: 1 day > > If you are opposed to the use of Auto-abandon, please propose an > alternative method to clean up the backlog of reviews on VPP master and > maintain a reasonably sized queue. > If you approve of the concept, please respond with a +1. > If you approve of the concept but don't like the configuration, please > respond with your preferred configuration. > > Lack of response will be interpreted as approval of the use of auto- > abandon with the proposed configuration ;) > > Thanks, > -daw- > > [0] dwallacelf@daw-server-2:~$ ssh -p 29418 gerrit.fd.io gerrit query > status:open project:vpp branch:master --format=JSON --no-limit | tail -1 > {"type":"stats","rowCount":636,"runTimeMilliseconds":1467,"moreChanges":fa > lse} > [1] https://gerrit.fd.io/r/c/vpp/+/1529 > [2] https://gerrit-review.googlesource.com/Documentation/config- > gerrit.html#changeCleanup > [3] https://gerrit-review.googlesource.com/Documentation/user-change- > cleanup.html#auto-abandon > > > > > >
-=-=-=-=-=-=-=-=-=-=-=- Links: You receive all messages sent to this group. View/Reply Online (#18627): https://lists.fd.io/g/vpp-dev/message/18627 Mute This Topic: https://lists.fd.io/mt/80169540/21656 Group Owner: vpp-dev+ow...@lists.fd.io Unsubscribe: https://lists.fd.io/g/vpp-dev/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
