ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dmitriy Setrakyan <dsetrak...@apache.org>
Subject Re: "Review workflow" changes to prevent "broken review" issues.
Date Tue, 06 Jun 2017 16:11:05 GMT
On Tue, Jun 6, 2017 at 7:40 AM, Anton Vinogradov <avinogradov@gridgain.com>
wrote:

> Igniters,
>
> Since we found that proposed approach can help,
> no one mind that I'll add text listed above to the wiki?
>

I don't think that we have an agreement yet. Again, I still don't think it
is fair for a contributor to find a committer that has a relevant area of
expertise. A contributor should feel free to ask any committer for a
review, but it should not be mandatory. I would rather have an existing
contributor or committer help with finding a reviewer.


>
> On Tue, Jun 6, 2017 at 1:19 PM, Anton Vinogradov <avinogradov@gridgain.com
> >
> wrote:
>
> > Dmitry,
> >
> > 1) See my initial email, it contains instruction how to find a reviewer.
> > And it's pretty easy to do when you have something to review (you did
> some
> > code changes).
> >
> > I want to add following to our wiki:
> >
> > "
> > Ask commiter to review changes.
> > Check affected file's git history to find commiter most likely able to
> > review changes.
> > In case it's hard to determine who's able to review by git history use
> > maintainers list presented above.
> > Add "review request" comment to the ticket starting with a commiter
> > username.
> >
> > for example: "[~avinogradov], Please review my changes."
> >
> > Commiter will gain notification and review your changes and/or find
> > another commiter to do this.
> >
> > Important: Each comment should be started with [~username].
> > "
> >
> > 2) It will be a huge help to the community!
> >
> > On Tue, Jun 6, 2017 at 1:12 PM, Dmitry Pavlov <dpavlov.spb@gmail.com>
> > wrote:
> >
> >> Anton,
> >>
> >>
> >> Thank you for explanation. Personal ask instead of group broadcast may
> >> really help. I understand the idea now.
> >>
> >>
> >> One argument against solution way 1) it may be not easy for contributor,
> >> especially newcomer, to find a right person.
> >>
> >>
> >> What do you think about way 2? Personally, I'm ready to help with
> analysis
> >> and assignment of these 66 tasks from next week.
> >>
> >>
> >>
> >> вт, 6 июн. 2017 г. в 12:57, Anton Vinogradov <avinogradov@gridgain.com
> >:
> >>
> >> > Dmitry Pavlov,
> >> >
> >> > There is *HUGE *difference between "Devlist, please review my changes"
> >> > and "Dmitry Pavlov, please review my changes".
> >> >
> >> > In case you're busy right now, you'll, most likely, ignore appeal to
> >> > devlist, but, I'm pretty sure, you'll check appeal to yourself.
> >> > Am I right?
> >> >
> >> > So, my idea is: appeal to devlist is a useless spam maker approach,
> but
> >> > appeal to person(s) works.
> >> >
> >> > On Tue, Jun 6, 2017 at 2:40 AM, Dmitriy Setrakyan <
> >> dsetrakyan@apache.org>
> >> > wrote:
> >> >
> >> > > Wow, we have 66 tickets waiting for review.... this is pretty bad.
> >> > > Something must definitely change.
> >> > >
> >> > > From my side, having a contributor shop around for a reviewer is not
> >> > really
> >> > > fair. However, I would support the idea of Apache Ignite community
> >> > electing
> >> > > a person responsible to find reviewers for all contributions.
> >> > >
> >> > > D.
> >> > >
> >> > > On Mon, Jun 5, 2017 at 11:23 AM, Dmitry Pavlov <
> dpavlov.spb@gmail.com
> >> >
> >> > > wrote:
> >> > >
> >> > > > 1) There is waiting for review list here
> >> > > > https://cwiki.apache.org/confluence/display/IGNITE/
> >> > > > Issues+waiting+for+review
> >> > > >
> >> > > > 2) some of contributions are supplemented by dev-list messages
> >> (please
> >> > > > review my PR…)
> >> > > >
> >> > > >
> >> > > > And these two tools sometimes do not help. I suppose it is because
> >> of
> >> > > > reviewers already have some things to do, but not because of
lack
> of
> >> > tool
> >> > > > support. Do you have other explanations?
> >> > > >
> >> > > >
> >> > > > But still, Igor’s suggestion to notify to dev list sounds
> >> reasonable to
> >> > > me.
> >> > > >
> >> > > >
> >> > > >
> >> > > > Anton, could it solve your requirement to know about issue needed
> to
> >> > > > review?
> >> > > >
> >> > > > пн, 5 июн. 2017 г. в 21:06, Igor Sapego <isapego@gridgain.com>:
> >> > > >
> >> > > > > By the way, there are emails being sent from Jira to devlist
> every
> >> > > > > time someone adds comment to ticket, or, for example, edits
its
> >> > > > > title which helps a lot to keep a track of ticket's state.
> >> > > > >
> >> > > > > But by some reason there is no such notification when ticket
> >> silently
> >> > > > > getting moved to "Patch available" state. I believe, that
would
> >> help
> >> > if
> >> > > > > there was a notification for that. Is it possible to configure?
> >> > > > >
> >> > > > > Best Regards,
> >> > > > > Igor
> >> > > > >
> >> > > > > On Mon, Jun 5, 2017 at 9:00 PM, Denis Magda <dmagda@apache.org>
> >> > wrote:
> >> > > > >
> >> > > > > > In general, I tend to agree with Anton that something
needs to
> >> be
> >> > > > changed
> >> > > > > > in this direction.
> >> > > > > >
> >> > > > > > How many of you flip through dev list, JIRA or GitHub
> >> notifications
> >> > > in
> >> > > > an
> >> > > > > > attempt to find tickets that demand your attention?
I bet the
> >> > > > percentage
> >> > > > > is
> >> > > > > > pretty low.
> >> > > > > >
> >> > > > > > To solve this issue I see two options:
> >> > > > > > 1) Proposed by Anton.
> >> > > > > > 2) Having a guy in the community who’ll keep an eye
on all the
> >> > > incoming
> >> > > > > > pull-requests shuffling them between committer in the
same way
> >> > > proposed
> >> > > > > in
> >> > > > > > 1.
> >> > > > > >
> >> > > > > > Personally, I’m for 1.
> >> > > > > >
> >> > > > > > —
> >> > > > > > Denis
> >> > > > > >
> >> > > > > > > On Jun 5, 2017, at 10:28 AM, Dmitry Pavlov <
> >> > dpavlov.spb@gmail.com>
> >> > > > > > wrote:
> >> > > > > > >
> >> > > > > > > Hi Anton,
> >> > > > > > >
> >> > > > > > >
> >> > > > > > > It is ok for me if it is advice and hint for faster
review,
> >> as it
> >> > > is
> >> > > > > now.
> >> > > > > > >
> >> > > > > > >
> >> > > > > > > We can periodically remind about this opportunity
at dev
> list
> >> or
> >> > in
> >> > > > the
> >> > > > > > > issue comments. We can remind that tasks in patch
available
> >> > status
> >> > > > may
> >> > > > > be
> >> > > > > > > reassigned by contributor.
> >> > > > > > >
> >> > > > > > >
> >> > > > > > > Looking from prospective of overall throughput:
it is not
> >> clear
> >> > for
> >> > > > me
> >> > > > > > how
> >> > > > > > > this process change will help.
> >> > > > > > >
> >> > > > > > >
> >> > > > > > > Best Regards,
> >> > > > > > >
> >> > > > > > > Dmitriy Pavlov
> >> > > > > > >
> >> > > > > > > пн, 5 июн. 2017 г. в 20:16, Anton Vinogradov
<av@apache.org
> >:
> >> > > > > > >
> >> > > > > > >> Vova,
> >> > > > > > >>
> >> > > > > > >> Contributors interested to make contributions
and I propose
> >> them
> >> > > to
> >> > > > > use
> >> > > > > > >> *same* strategy as we (people inside the community)
use.
> >> > > > > > >> "-1" will not solve this issue, but my "tips"
will.
> >> > > > > > >>
> >> > > > > > >> Dmitry,
> >> > > > > > >>
> >> > > > > > >> The main problem here is that nobody notified
that someone
> is
> >> > > > waiting
> >> > > > > > for
> >> > > > > > >> the review.
> >> > > > > > >> It's not a problem for me to provide tips
or to make
> review,
> >> but
> >> > > > it's
> >> > > > > > >> problem to periodically check is there somebody
waiting.
> >> > > > > > >>
> >> > > > > > >> Guys,
> >> > > > > > >> Let's try this approach, and I'm pretty sure
it will help.
> >> > > > > > >>
> >> > > > > > >> On Mon, Jun 5, 2017 at 7:58 PM, Dmitry Pavlov
<
> >> > > > dpavlov.spb@gmail.com>
> >> > > > > > >> wrote:
> >> > > > > > >>
> >> > > > > > >>> Hi Igniters, Anton,
> >> > > > > > >>>
> >> > > > > > >>> Let’s imagine that development process
as a chain of
> >> production
> >> > > > > stages
> >> > > > > > >>> 1) Developing patch by contributor
> >> > > > > > >>> 2) Review changes by committer
> >> > > > > > >>>
> >> > > > > > >>> Reviews waiting too long time to be done
at stage 2 may
> >> > indicate
> >> > > > that
> >> > > > > > >> speed
> >> > > > > > >>> (potential throughput) of step 2 is less
than throughput
> at
> >> > step
> >> > > 1.
> >> > > > > > T2<T1
> >> > > > > > >>>
> >> > > > > > >>> In terms of this model (inspired by Goldratt’s
Theory of
> >> > > > Constraints
> >> > > > > > >>> (TOC)), I have a question:
> >> > > > > > >>> Will this responsibility movement (to
find appropriate
> >> reviewer
> >> > > to
> >> > > > > > >>> contributor) help us to increase overall
throughput?
> >> > > > > > >>>
> >> > > > > > >>> If we agree constraint in terms of TOC
is throughput T2, I
> >> > > suggest
> >> > > > > > >>> following steps
> >> > > > > > >>> - Increase the throughput T2 of the committers
> >> > > > > > >>> - Reduce the load on the committers by
improve quality of
> >> code
> >> > > > after
> >> > > > > > >> stage
> >> > > > > > >>> 1 given to review (pre review by non-committer,
automatic
> >> > review,
> >> > > > > code
> >> > > > > > >>> inspections)
> >> > > > > > >>>
> >> > > > > > >>> Best Regards,
> >> > > > > > >>> Dmitriy Pavlov
> >> > > > > > >>>
> >> > > > > > >>>
> >> > > > > > >>> пн, 5 июн. 2017 г. в 18:28, Anton
Vinogradov <
> av@apache.org
> >> >:
> >> > > > > > >>>
> >> > > > > > >>>> Igniters,
> >> > > > > > >>>>
> >> > > > > > >>>> Currently, according to
> >> > > > > > >>>>
> >> > > > > > >>>> https://cwiki.apache.org/confluence/display/IGNITE/How+
> >> > > > > > >>> to+Contribute#HowtoContribute-SubmittingforReview
> >> > > > > > >>>> ,
> >> > > > > > >>>> contributor can ask for review by
moving ticket to PATCH
> >> > > AVAILABLE
> >> > > > > > >> state.
> >> > > > > > >>>>
> >> > > > > > >>>> And, as far as I can see, this cause
broken tickets
> issue.
> >> > > > > > >>>> Contributor can wait somebody who'll
review his changes
> >> for a
> >> > > > month
> >> > > > > or
> >> > > > > > >>> even
> >> > > > > > >>>> more.
> >> > > > > > >>>>
> >> > > > > > >>>> I propose to change workflow and *make
contributor
> >> responsible
> >> > > to
> >> > > > > find
> >> > > > > > >>>> reviewer*.
> >> > > > > > >>>> It's pretty easy to find a person
able to review changes
> in
> >> > most
> >> > > > of
> >> > > > > > >>> cases.
> >> > > > > > >>>>
> >> > > > > > >>>> 1) You can check git history of files
you modified and
> find
> >> > > > persons
> >> > > > > > >> with
> >> > > > > > >>>> expertise in this code
> >> > > > > > >>>> 2) In case you have problems with
such search you can
> >> always
> >> > use
> >> > > > > > >>>> maintainers list (
> >> > > > > > >>>>
> >> > > > > > >>>> https://cwiki.apache.org/confluence/display/IGNITE/How+
> >> > > > > > >>> to+Contribute#HowtoContribute-ReviewProcessandMaintainers
> >> > > > > > >>>> )
> >> > > > > > >>>>
> >> > > > > > >>>> Thoughts?
> >> > > > > > >>>>
> >> > > > > > >>>
> >> > > > > > >>
> >> > > > > >
> >> > > > > >
> >> > > > >
> >> > > >
> >> > >
> >> >
> >>
> >
> >
>

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