ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dmitry Pavlov <dpavlov....@gmail.com>
Subject Re: "Review workflow" changes to prevent "broken review" issues.
Date Tue, 06 Jun 2017 10:12:06 GMT
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