Test-infra: [mungegithub] ping inactive reviewers and reassign

[foxish]

_From @foxish on July 22, 2016 21:0_

Formerly part of issue #869

• Ping the PR due to lack of inactivity, notifying either the PR author or the assignee, depending on who last touched the PR (keep track of whose court it's in).
• Create a way to postpone the next ping for a fixed amount of time (day, week, month, next release cycle).
• Reassign PRs whose assignees never responded.

Additionally:

• label stale PRs.

_Copied from original issue: kubernetes/contrib#1422_

[foxish]

_From @grodrigues3 on August 5, 2016 22:4_

This is related to #1402. I think that one already pings due to inactivity and also notifies the "involved Users". We might be able to add additional granularity to the notifications by checking for keywords like PTAL or paying attention to the last action (e.g new commit)

[foxish]

@grodrigues3 Good point. It does seem related and we could repurpose parts of that munger. As part of this issue, it seems like we could tackle https://github.com/kubernetes/kubernetes/issues/30034 as well. We could do the labeling and the pings. The close-stale-pr munger has the duration between pings set to a very large number I think (30 days?). It would make sense to parameterize that and decide on the right duration till the first ping and between successive pings.

[grodrigues3]

@crimsonfaith91 this is another good one.

@kubernetes/sig-contributor-experience-misc

[crimsonfaith91]

noted! i will have a look at it. :)

[crimsonfaith91]

@foxish @grodrigues3
After reading descriptions of issues involved, I am thinking about implementing two sub-features:
(1) check for keyword PTAL in new comments: what should the bot do when it found the keyword?
(2) parameterize ping duration

[fejta]

Ptal could direct reviewers to the PR dashboard?

I'm not super sure this is necessary though now that we have gubernator, which already has the concept of whose turn it is to act on a pr

[crimsonfaith91]

@fejta Good point! i will discuss it with Garrett and Anirudh. :)

[foxish]

I think we see enough bot comments as it is, and adding more noise with pings is not desirable. I think we should point people to the dashboard as @fejta said and that will go a long way in ensuring that things don't fall off their plate and get lost completely.

[grodrigues3]

I can see not pinging, but I think reassigning after a prolonged period. And maybe leaving a comment indicating how anyone can triage the issue (e.g /area <someLabel> or mentioning a sig or just closing /close)

[fejta]

Giving the author some way of flagging a PR for needing attention would help: https://github.com/kubernetes/test-infra/issues/1631

[crimsonfaith91]

I think we can unassign stale PRs without pinging anybody if the PRs do not have any activity for a long period (e.g., 90 days). The unassigned PRs can then be labelled accordingly (e.g., inactive).

Will leaving a bot comment for an inactive PR be useful? The comment can be useful if it tags at least someone. Otherwise, the comment is probably not to be read by anyone due to the PR being stale. Perhaps a better way is to notify _at least_ one of the assignees / PR author that the PR is unassigned.

[grodrigues3]

blunderbuss looks at every PR so if you unassign it will reassign to someone else from the owners files
https://github.com/kubernetes/test-infra/blob/master/mungegithub/mungers/blunderbuss.go#L172

There is a nonzero chance that you reassign the same person again. Can we do better?

Also, 90 days is way too long. I think less than a week is reasonable (maybe 3 or 4 days). No need for inactive label in my opinion.

Perhaps the bot can comment with link an appropriate OWNERS file and explain to the PR author how they can manually assign to someone from there (i.e. /assign

[crimsonfaith91]

As blunderbuss will reassign to another owner, inactive label is
unnecessary. The algorithm can be changed to ensure there is no chance an
assignee will be reassigned again.

Having a comment with a link to the OWNERS file will be helpful. So do we
plan to ping only PR author in this case?

On May 10, 2017 6:03 PM, "grodrigues3" [e-mail] wrote:

blunderbuss looks at every PR so if you unassign it will reassign to
someone else from the owners files
https://github.com/kubernetes/test-infra/blob/master/mungegithub/mungers/
blunderbuss.go#L172

There is a nonzero chance that you reassign the same person again. Can we
do better?

Also, 90 days is way too long. I think less than a week is reasonable
(maybe 3 or 4 days). No need for inactive label in my opinion.

Perhaps the bot can comment with link an appropriate OWNERS file and
explain to the PR author how they can manually assign to someone from there
(i.e. /assign

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/test-infra/issues/1631#issuecomment-300653569,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ANRtXtZRKN6gLiWZrqUqWJ5Ev3A4hOHyks5r4l57gaJpZM4Lp6mB
.

[fejta]

Agree with Garrett that we do not need to wait a quarter, that's WAY too long. I would say the bot should /assign @additional-person every week of inactivity :)

I would prefer it just adds an additional person and not unassign the original person. I may be on vacation or something.

[crimsonfaith91]

If the PR is inactive for several weeks, do we add an additional person every week?

[grodrigues3]

I would prefer it just adds an additional person and not unassign the original person. I may be on vacation or something.

I'm concerned that if multiple people are responsible for reviewing a PR, no one will be responsible

[crimsonfaith91]

In this case, perhaps we can unassign the original person.

[lavalamp]

Agree w/ Garrett

On Thu, May 11, 2017 at 11:16 AM, grodrigues3 [e-mail]
wrote:

I would prefer it just adds an additional person and not unassign the
original person. I may be on vacation or something.

I'm concerned that if multiple people are responsible for reviewing a PR,
no one will be responsible

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/test-infra/issues/1631#issuecomment-300873599,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAnglkKHsiARCgPPzAbnLnMvSjTbXmY1ks5r41BngaJpZM4Lp6mB
.

[crimsonfaith91]

Will the steps below suffice to solve the issue?
(1) for every week of inactivity, add another person and unassign original person
(2) change blunderbuss algorithm so that there is no chance that same person is reassigned again

I think a munger works best. I can work on this. Any opinion? :)

[grodrigues3]

SGTM. I wonder if there's anyway we can also consider number of reviewers each person is currently assigned to when choosing reviewer. @rmmh does gubernator have this info?

[lavalamp]

That sounds like it's worth trying. A week might be a bit long, but who
knows.

I think it should also take into account how much other stuff is assigned
to the reviewer, but that's a separate work item.

On Fri, May 12, 2017 at 10:30 AM, Jun Xiang Tee [e-mail]
wrote:

Will the steps below suffice to solve the issue?
(1) for every week of inactivity, add another person and unassign original
person
(2) change blunderbuss algorithm so that there is no chance that same
person is reassigned again

I think a munger works best. I can work on this. Any opinion? :)

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/test-infra/issues/1631#issuecomment-301137890,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAnglk3ZcF4jXsA1R_KqMeWdLq8oIqXjks5r5JcYgaJpZM4Lp6mB
.

[crimsonfaith91]

/assign @crimsonfaith91

[fejta]

How can I disable this in my repos? I absolutely do not want some bot unassigning me.

And I will be highly surprised if there is a positive correlation between number of assignees and latency. I expect the correlation to be negative -- meaning a PR with 3 assignees will get attention much faster than 1 assignee.

This is how every project inside of Google handles this, for example.

[fejta]

IMO a better way to handle this will be for the bot to leave a comment encouraging the PR author to /assign a new person and /unassign the current reviewer. For example:

Sorry it is taking a long time for @fejta to review your PR. This person may be on vacation or otherwise occupied. To expedite a review, please /assign @crimsonfaith91 and consider /unassign @fejta.

This empowers the person with the means to move forward in a way that will minimize the chances for side effects from a low-quality automated decision.

[crimsonfaith91]

@fejta
Sorry, i accidentally voted down when i wanted to vote up...

What will happen if both PR author and reviewers are being inactive? Will the comment be useful in this case? I think it is not likely to happen though.

[cblecker]

If the PR author is inactive, then the PR itself as a whole will time out and close after 90 days (this already happens).

[fejta]

IMO if the PR author doesn't care enough about their pr to /assign @someone it probably isn't worth bothering that someone. We can just wait for the current assignee to get to it.

[crimsonfaith91]

Noted!

I will write a new munger that creates the comment after a week of inactivity. The munger will use Blunderbuss algorithm to suggest a new reviewer who is _not_ the original reviewer.

Is there anything that the munger needs to handle?

[lavalamp]

Erick's suggestion sounds fine to me, too.

On Fri, May 12, 2017 at 3:07 PM, Jun Xiang Tee [e-mail]
wrote:

Noted!

I will write a new munger that creates the comment after a week of
inactivity. The munger will use Blunderbuss algorithm to suggest a new
reviewer who is not the original reviewer.

Is there anything that the munger needs to handle?

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/test-infra/issues/1631#issuecomment-301196756,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAnglqpR-cZslOhpDtQLVpa8koLds9C_ks5r5NgFgaJpZM4Lp6mB
.

[apelisse]

I'd be interested to know how you're going to handle corner cases?

Blunderbuss algorithm to suggest a new reviewer who is not the original reviewer.

It should probably consider also other unassigned people. And what if you don't have any other good reviewer?

[crimsonfaith91]

I plan to use the algorithm below. From my understanding, the reviewers are assignees in this case. Correct me if I am wrong.

(1) get all current assignees for the PR
(2) get potential owners of the PR using Blunderbuss algorithm (calling getPotentialOwners() function)
(3) filter out current assignees from the potential owners
(4) if there is no another good reviewer, the bot will encourage the PR author to ping all existing assignees
(5) otherwise, select a new reviewer using Blunderbuss algorithm (calling selectMultipleOwners() function with number of assignees parameter of one)

Based on @fejta feedback, the bot will leave a comment encouraging the PR author to unassign someone. The bot will not unassign people automatically. If someone is unassigned, the algorithm above _will_ consider him/her too. This is because step 2 and 3 are performed every time the munger runs.

It is not efficient to run step 2 and 3 everytime. Will there be a memoization mechanism for mungers?

Tell me if I am on the wrong track. Thanks!

[grodrigues3]

Yeah, that sounds alright.

Only feedback is explain why the new assignee got assigned despite there already being assignees on the issue/pr. (e.g. We're adding you because @ grodrigues3 is being unresponsive). OK, maybe that's too harsh, but some context would be helpful :)

[cblecker]

Another thought: in the same vein as "[if] multiple people are responsible for reviewing a PR, no one will be responsible", right now we're assigning two reviewers right off the bat. Perhaps if we have a better way of suggesting alternate reviewers (as suggested above), we should only assign 1 off the top?

[crimsonfaith91]

should i change number of reviewers from 2 to 1?

[fejta]

We should only go back to that when someone provides evidence it works better.

I will be utterly astonished if a single assignee has lower tail latency than multiple. We used to do this and we went away from it because it does not work.

[crimsonfaith91]

noted! i will leave it at 2 for now until proven otherwise. thanks! :)

[grodrigues3]

@crimsonfaith91 can we mark this closed?

[crimsonfaith91]

sure!
/close