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> 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> wrote:
>>>>>
>>>>> +1
>>>>>
>>>>> ben
>>>>>
>>>>> -----Original Message-----
>>>>> From: 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 (#18621): https://lists.fd.io/g/vpp-dev/message/18621
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]
-=-=-=-=-=-=-=-=-=-=-=-
