drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Zelaine Fong <zf...@maprtech.com>
Subject Re: [PROPOSAL] Apache Jira Workflow for Code Reviews
Date Mon, 05 Dec 2016 16:32:53 GMT
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 <zfong@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 <progers@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.apache@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 <amansinha@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 <zfong@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
>> >>>
>>
>>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message