"Skip next time" reviewer got stuck at top of "Next in the reviewer rotation", "skip" setting never cleared

[ietf-svn-bot]

owner:jennifer@painless-security.com resolution_fixed type_defect | by mahoney@nostrum.com

---

I clicked "skip next time" for a reviewer because she had a heavy workload. Her name appeared in the "Next in the reviewer rotation" list on the Review Assignments email, but she did not show up as a reviewer selection in the drop-down list on the "Manage unassigned open review requests" page even after she should have been added back into the queue (she was set to skip 1 and the queue had wrapped a few times).

I didn't realize that she had gotten "stuck" until she asked what had happened to her assignments. I cleared the issue by setting her "Skip next assignments" count to 0 on her "Change reviewer settings" page.

It appears that "Skip next assignments" has an issue with clearing its setting automatically.

---

Issue migrated from trac:3038 at 2022-03-04 07:56:16 +0000

[ietf-svn-bot]

@rjsparks@nostrum.com changed status from new to accepted

[ietf-svn-bot]

@rjsparks@nostrum.com changed priority from n/a to major

[ietf-svn-bot]

@rjsparks@nostrum.com commented

---

There is tension in the code around determining exactly when a reviewer is "skipped". This is true with all queue strategies, but we'll focus on the LRU strategy for now.

The assignment form suggests reviewers - if it chooses not to suggest a reviewer that it would otherwise suggest because of skip-next, we're in the halo of when skip-next should be decremented. But it's not clear that it should be decremented unless an assignment to next person in the queue is actually made.

So, I think the logic in the POST should be to look at the prefix of the queue that actually gets assignments. If there is anyone in that prefix that didn't get an assignment who had a positive skip-next, decrement it.

I think this is a reasonable algorithm to apply to both of the queue strategies we currently have implemented, and it might clear up some of the issues Tero's been having with the alphabetic round-robin strategy.

[ietf-svn-bot]

@mark@painless-security.com commented

---

I've been looking at the code here (ietf/review/policies.py:66-119, in update_policy_state_for_assignment), which already seems to loop over the full in-order list of the reviewer rotation. I'm still trying to determine what would lead to the reviewer being left eternally with skip_next set to 1.

[ietf-svn-bot]

@mark@painless-security.com changed status from accepted to assigned

[ietf-svn-bot]

@mark@painless-security.com changed owner from `` to mark@painless-security.com

[ietf-svn-bot]

@mark@painless-security.com commented

---

Who was the reviewer that was stuck with skip_next greater than 0?

[ietf-svn-bot]

@mark@painless-security.com commented

---

During my investigation of this issue, I realized that traversing the full in-order reviewer rotation list does have an edge case of wrong behavior... if:

• you have a list of a number of people (say, "A", "B", and "C")
• the first Person ("A") on the list has skip_next set to 2
• the requested reviewer has skip_next set to 0
• every other person has skip_next set to 1

... then the system will double-decrement the value of skip_next for the first person. In order, it will:

• decrement the value of skip_next of the first person on the list ("A") down to 1
• skip over the requested reviewer
• decrement the value of skip_next of every other person on the list ("C") down to 0
• loop back over from the beginning, setting the skip_next of the first person ("A") down to 0
• skip over the requested reviewer
• set the following reviewer candidate on the rotation list to be the "next reviewer".

Effectively, in this (odd) case the first reviewer on the list will have their skip_next value decremented twice.

[ietf-svn-bot]

@mahoney@nostrum.com commented

---

Replying to ietf-svn-conversion/datatracker#3038 (comment:6):

Who was the reviewer that was stuck with skip_next greater than 0?

Meral Shirazipour

[ietf-svn-bot]

@jennifer@painless-security.com changed owner from mark@painless-security.com to jennifer@painless-security.com

[ietf-svn-bot]

@jennifer@painless-security.com commented

---

Regarding Mark's corner case, I think the double-decrement of 'A' is likely intentional. That person was considered once and skipped (when assigning to 'B'), then considered a second time and skipped again (when looking to assign the "next reviewer"). So far so good, at least to the extent that I understand it.

It looks to me like a bug is still there, though: if 'add_skip' is False, 'B' should also be the "next reviewer" because they should not be skipped. If 'add_skip' is True, then 'C' is correctly made "next reviewer," but 'B' will come out with 'skip_next = 1' when it should be 0 since they were skipped.

I don't think that's causing the bug seen here, though.

[ietf-svn-bot]

@jennifer@painless-security.com commented

---

I've spent some time looking into this and have not yet found an explanation for the behavior.

One thing that stands out: you say that Meral's name was listed in the "Next in the reviewer rotation" part of the review assignments email, but not listed in the drop-down to assign as a reviewer. Are you certain that she did not appear in the drop-down and wasn't just far down the list?

I ask because based on what I see in the database, I don't see any path that would explain that. It is possible for a name to appear in the email but not the drop-down, but only if the person is marked as unavailable. I don't see any unavailable periods for that user.

The current codebase would also keep someone out of the drop-down if they had previously rejected or not responded to a request for the document you were managing, but that code was not merged until a few weeks after this issue was filed (and I think you'd sorted this out a few weeks before that?)

If you happen to know any particular documents where the name did not appear in the drop-down, that would be helpful. Otherwise, I'm struggling to reproduce this.

[ietf-svn-bot]

@jennifer@painless-security.com commented

---

One other thought - the drop-down vs email is puzzling, but might be a distinct issue from the 'skip_next' not updating. When assigning reviewers, are you routinely assigning the first name in the drop-down?

If you're not accepting the reviewer the LRU policy determined is next in line, then the skip_next updates are ignored. This is the most promising explanation I can find so far.

[ietf-svn-bot]

@mahoney@nostrum.com commented

---

I don't remember if Meral was listed further down on the drop-down list. I almost always select the first person in the drop-down list.

[ietf-svn-bot]

@jennifer@painless-security.com commented

---

Ok, I think I've found it - and it's a lot simpler than I was thinking. The code that decides whether or not to update skip_next runs after the new assignment is made. For the LRU policy, making the assignment immediately moves the assignee to the end of the rotation. As a result, the assignment is never "in order" and skip_next is never updated.

I think this is in addition to the subtler bugs.