Hi Paul,
My original motivation for this proposal is to improve the stability and
availability of the CI system by not abusing it through lack of queue
maintenance.
The situation exists today because there is fundamentally a shortage of
committer bandwidth to complete code reviews. This is exacerbated by
the poor signal-to-noise ratio of the gerrit queue due to the size as
well as by the growing reluctance to merge patches for which a committer
is not a maintainer. [Thanks Benoit for pointing that out.]
IMHO, it is the responsibility of both the author of a patch and the
maintainers/committers in the community to complete the review/merge
cycle. As a committer, I am personally reluctant to merge patches more
than 30 days old because they have not been tested with the latest
code. A best practice would be for an author rebase any patches that
have not been verified in a couple of weeks. This not only has the
benefit of re-testing the patch with the latest merged code, it moves
the patch to the top of the gerrit queue where it is most visible.
In the past, I worked on an open source project which had a 7 day
auto-abandon policy. It took me a while to get used to it, but it
worked quite effectively. Feedback on patches was timely and the queue
only contained patches which were actively ready for review. For VPP, I
think that 7 days is way too short but as a maintainer of the CI, I
believe it is important find a way to automatically protect the CI
system which has been overly fragile.
To be perfectly honest, other than Andrew's proposal to tweak the
auto-abandon parameters, I have not heard another solution that solves
the problem of cleaning up the current queue and limiting the size of
the queue in the future. Is anyone going to volunteer to manually
review/abandon 600+ gerrit changes? Auto-assigning maintainers to
gerrit changes is a separate issue. Please make a proposal to fix that
in its own thread and I will help to get that implemented.
I fully understand that enabling auto-abandon in gerrit results in a
bias towards preserving committers' time over submitters' time, but I
think it is worth trying as it has the potential provide a positive
benefit for both the stability of the CI and the management of the
gerrit queue.
Thanks,
-daw-
On 1/29/2021 9:28 AM, Paul Vinciguerra wrote:
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/
<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
<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 <mailto:ayour...@gmail.com>> wrote:
Dave,
On 28 Jan 2021, at 16:59, Dave Wallace <dwallac...@gmail.com
<mailto: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>
<mailto: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) vialists.fd.io <http://lists.fd.io>
<bganne=cisco....@lists.fd.io> <mailto:bganne=cisco....@lists.fd.io> wrote:
+1
ben
-----Original Message-----
From:vpp-dev@lists.fd.io <mailto:vpp-dev@lists.fd.io> <vpp-dev@lists.fd.io>
<mailto:vpp-dev@lists.fd.io> On Behalf Of Dave Wallace
Sent: mercredi 27 janvier 2021 22:50
To:vpp-dev@lists.fd.io <mailto: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 29418gerrit.fd.io
<http://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
<https://gerrit.fd.io/r/c/vpp/+/1529>
[2]https://gerrit-review.googlesource.com/Documentation/config
<https://gerrit-review.googlesource.com/Documentation/config>-
gerrit.html#changeCleanup
[3]https://gerrit-review.googlesource.com/Documentation/user-change
<https://gerrit-review.googlesource.com/Documentation/user-change>-
cleanup.html#auto-abandon
-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#18631): https://lists.fd.io/g/vpp-dev/message/18631
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]
-=-=-=-=-=-=-=-=-=-=-=-
