Option 2 sounds good. On Mon, Dec 5, 2016 at 8:32 AM, Zelaine Fong <zf...@maprtech.com> wrote:
> Any other thoughts or comments on this? > > From the limited number of "votes", it seems like the preference is for > option 2 -- keeping the Assignee field set to the fixer and using other > fields to denote the code reviewer and readiness for commit. > > If I don't hear any other opinions, I'll work with others to update the > wiki page to make this process change. > > Thanks. > > -- Zelaine > > On Tue, Nov 29, 2016 at 11:59 AM, Zelaine Fong <zf...@maprtech.com> wrote: > > > 1) Yes, we don't have a good system for this. Committers can > > watch/monitor and do the assignment, as you've suggested. > > > > 2) If we go with proposal #2, then yes, adding a comment when the > Reviewer > > assignment is done would be needed to trigger an email notification. Or > we > > could make the Reviewer a watcher on the issue. In either case, the > > explicit additional step is required. > > > > Regarding adding new status states to Apache Jira -- I'm not sure how > hard > > that is. I recall discussion about attempts being made to add new fields > > into Apache Jira. But I'm not sure about states. Any one have > experience > > with this? If it's easy to do, I think that's a better way of tracking > the > > new state change that proposal #2 needs. > > > > -- Zelaine > > > > > > On Tue, Nov 29, 2016 at 11:02 AM, Paul Rogers <prog...@maprtech.com> > > wrote: > > > >> Sounds like #2 has a clear lead. Couple of suggestions. > >> > >> 1. How do we know the reviewer to specify? In many cases, we informally > >> know a likely person. Community members may not be sure. Or, can a > >> committer watch for such cases and do the assignment? > >> > >> 2. As it turns out, JIRA does not send an e-mail notification when > >> assigning someone as a Reviewer. Instead, perhaps include a brief > comment, > >> mentioning that person, to force JIRA to alert the Reviewer that they > have > >> a new item. > >> > >> As a follow up, anyone know if we can lobby Apache to add status states? > >> That is, to add the states that Julian suggested? Typical workflow: > Open -> > >> In Progress -> Reviewable -> Ready to Commit -> Resolved. > >> > >> (Actually, if we include QA activities, the flow should be Resolved —> > >> Verified. But, that is a different discussion.) > >> > >> The list of states (visible from clicking Resolve), includes some odd > >> items such as “Staged” and “REMIND”, which seem like specialized states > >> added for ad-hoc purposes... > >> > >> Thanks, > >> > >> - Paul > >> > >> > On Nov 29, 2016, at 9:23 AM, Julian Hyde <jhyde.apa...@gmail.com> > >> wrote: > >> > > >> > I like 2 also. > >> > > >> > Is the following variant of 2 possible in JIRA? Make the "ready to > >> commit" flag into a status. Thus status changes from "open" to "in > >> progress" to "reviewable" to "ready to commit" (or "approved"). > >> > > >> > The inability to search by assignee should not be a huge problem - it > >> is in the interests of the project to ensure that the number of cases in > >> "reviewable" or "ready to commit" state at any moment is small. > >> > > >> > And furthermore, the author of a bug often doesn't know or care who > >> will review or commit it. So the very notion of an assignee is suspect. > >> > > >> > Julian > >> > > >> >> On Nov 29, 2016, at 07:53, Aman Sinha <amansi...@apache.org> wrote: > >> >> > >> >> My preference is for #2 because of the reasons you mention. For #1 > my > >> >> recollection is the DrillCommitter was used at a time when there were > >> only > >> >> couple of committers. Today there are many. Changing to > >> DrillCommitter > >> >> will leave ambiguity about which committer should look at it. > >> >> Additionally, I feel that today we are not using the field > 'Reviewer' > >> for > >> >> its correct purpose. This argues for #2. > >> >> > >> >> One note about #2 worth mentioning is that folks would have to change > >> their > >> >> search criteria to 'assigned *OR reviewer*' to find out what is on > >> their > >> >> plate. > >> >> > >> >>> On Mon, Nov 28, 2016 at 4:51 PM, Zelaine Fong <zf...@maprtech.com> > >> wrote: > >> >>> > >> >>> I'd like to propose a small change to the current process for code > >> reviews > >> >>> in Apache Drill. We need this change because non-committers are > >> becoming > >> >>> more involved in code reviews. > >> >>> > >> >>> The current process is described at > >> >>> https://drill.apache.org/docs/apache-drill-contribution- > >> >>> guidelines/#step-3:-get-your-code-reviewed-and-committed-to- > >> the-project. > >> >>> It assumes that the individuals doing code reviews are code > >> committers. > >> >>> Here's a brief summary of the process: > >> >>> > >> >>> *Current Process:* > >> >>> Currently, the workflow is based on updating the Status and Assignee > >> >>> fields. When a pull request has been posted and is ready for > review, > >> the > >> >>> issue status is changed to "Reviewable" and the Assignee is changed > >> to the > >> >>> designated code reviewer. If the review is completed and the > >> reviewer is a > >> >>> committer, the reviewer/committer will commit the change. If the > code > >> >>> reviewer has comments that require the contributor to address, the > >> issue > >> >>> status changes back to "In Progress" and the Assignee is changed > back > >> to > >> >>> the contributor. > >> >>> > >> >>> *Proposed Change:* > >> >>> The proposed change is to address the case where another step may be > >> needed > >> >>> to commit changes, if the changes have been reviewed by a > >> non-committer. > >> >>> Here are a couple of proposals on how to address this. In both > >> cases, once > >> >>> a review has been satisfactorily completed on a pull request, a > >> committer > >> >>> will take over, bless the changes based on the prior review > comments, > >> do a > >> >>> +1 on the patch, and then commit. > >> >>> > >> >>> > >> >>> *Proposal #1:*When a fix is ready to be reviewed, set the Assignee > to > >> >>> "DrillCommitter". > >> >>> > >> >>> Pros: Simple. User "DrillCommitter" already exists. > >> >>> Cons: Similar to changing the Assignee to the code reviewer, you > lose > >> track > >> >>> of who is the original contributor of an issue. Yes, it's in the > >> comment > >> >>> history, but there is no automated/easy way to get this info. > >> >>> > >> >>> *Proposal #2:* > >> >>> Leave the Assignee field unchanged. This is also means changing the > >> >>> current process of changing the Assignee to the code reviewer. > >> Instead, > >> >>> > >> >>> 1) Continue to set the Status to "Reviewable" to indicate that the > >> fix is > >> >>> ready for review, but set the Reviewer field to the designated code > >> >>> reviewer. > >> >>> 2) If changes are needed after code review, change the status of the > >> issue > >> >>> back to "In Progress" as today. But again, don't change the > Assignee. > >> >>> 3) If the changes are acceptable, the reviewer applies the > >> >>> "ready-to-commit" tag on the Jira. > >> >>> > >> >>> Pros: Maintains Assignee field > >> >>> Cons: Requires querying for the tag "ready-to-commit" to find > >> commit-ready > >> >>> fixes. > >> >>> > >> >>> Please comment with your preferences for proposal 1 or 2, or if you > >> have > >> >>> other suggestions. > >> >>> > >> >>> Thanks. > >> >>> > >> >>> -- Zelaine > >> >>> > >> > >> > > >
