[openstack-helm] would like to discuss review turnaround time
Pete Birley
petebirley+openstack-dev at gmail.com
Wed Feb 20 01:13:41 UTC 2019
Hi,
I must apologize for not responding to this sooner. I feel some irony
in that I, and many of the core review team in OpenStack-Helm, never
been that great at OpenStack mailing lists, but are very active on
IRC. To address the email that Jay sent to start this thread I've
attached some comments.
> There are just some example patch sets currently under review stage.
>
> 1. https://review.openstack.org/#/c/603971/ >> this ps has been discussed
> for its contents and scope. Cloud you please add if there is anything else
> we need to do other than wrapping some of commit message?
Last feedback was from myself on the 15th Jan, two weeks before the
date of the email thread that started this conversation - I really
like the PS, and see it has great value for operators of OSH, however,
we really need to gate this, as we do all other components in
OpenStack-Helm infra. Once we have a path there I see no reason not to
merge, as my overly happy +1 wf finger has shown over the last few
weeks.
> 2. https://review.openstack.org/#/c/633456/ >> this is simple fix. how can
> we make core reviewer notice this patch set so that they can quickly view?
This was merged within 48 hours of the initial commit, so I hope had
an acceptable turnaround. Please feel free to add a reviewer via
gerrit to any PS you think would benefit from their eye.
> 3. https://review.openstack.org/#/c/625803/ >> we have been getting
> feedbacks and questions on this patch set, that has been good. but
> round-trip time for the recent comments takes a week or more. because of
> that delay (?), the owner of this patch set needed to rebase this one
> often. Will this kind of case be improved if author engages more on irc
> channel or via mailing list to get feedback rather than relying on gerrit
> reviews?
This PS has indeed suffered from review stagnation, though this also
highlights that we need to rely on more than just the core reviewers
to provide meaningful feedback on patchsets. This PS is quite tricky
to digest, as it touches every chart in the repo, and has changed
direction several times in its lifespan. Here I think both a gentle
nudge via gerrit would provide some assistance.
Moving forward I'll also include announcements of the meeting agenda,
and links to the logs in a weekly email for the project, which should
hopefully allow easier engagement for those that find IRC a less
convenient communication mechanism.
Cheers
Pete
On Tue, 19 Feb 2019 at 8:46 AM, Jaesuk Ahn <bluejay.ahn at gmail.com> wrote:
>
> It has been several weeks without further feedback or response from openstack-helm project members.
> I really want to hear what others think, and start a discussion on how we can improve together.
>
>
>
> On Thu, Jan 31, 2019 at 4:35 PM Jaesuk Ahn <bluejay.ahn at gmail.com> wrote:
>>
>> Thank you for thoughtful reply.
>>
>> I was able to quickly add my opinion on some of your feedback, not all. please see inline.
>> I will get back with more thought and idea. pls note that we have big holiday next week (lunar new year holiday), therefore, it might take some time. :)
>>
>>
>> On Wed, Jan 30, 2019 at 9:26 PM Jean-Philippe Evrard <jean-philippe at evrard.me> wrote:
>>>
>>> Hello,
>>>
>>> Thank you for bringing that topic. Let me answer inline.
>>> Please note, this is my personal opinion.
>>> (No company or TC hat here. I realise that, as one of the TC members
>>> following the health of the osh project, this is a concerning mail, and
>>> I will report appropriately if further steps need to be taken).
>>>
>>> On Wed, 2019-01-30 at 13:15 +0900, Jaesuk Ahn wrote:
>>> > Dear all,
>>> >
>>> > There has been several patch sets getting sparse reviews.
>>> > Since some of authors wrote these patch sets are difficult to join
>>> > IRC
>>> > meeting due to time and language constraints, I would like to pass
>>> > some of
>>> > their voice, and get more detail feedback from core reviewers and
>>> > other
>>> > devs via ML.
>>> >
>>> > I fully understand core reviewers are quite busy and believe they are
>>> > doing
>>> > their best efforts. period!
>>>
>>> We can only hope for best effort of everyone :)
>>> I have no doubt here. I also believe the team is very busy.
>>>
>>> So here is my opinion: Any review is valuable. Core reviewers should
>>> not be the only ones to review patches
>>> The more people will review in all of the involved companies, the more
>>> they will get trusted in their reviews. That follows up with earned
>>> trust by the core reviewers, with eventually leads to becoming core
>>> reviewer.
>>
>>
>> This is a very good point. I really need to encourage developers to at least cross-review each other's patch set.
>> I will discuss with other team members how we can achieve this, we might need to introduce "half-a-day review only" schedule.
>> Once my team had tried to review more in general, however it failed because of very limited time allowed to do so.
>> At least, we can try to cross-review each other on patch sets, and explicitly assign time to do so.
>> THIS will be our important homework to do.
>>
>>
>>>
>>>
>>> I believe we can make a difference by reviewing more, so that the
>>> existing core team could get extended. Just a highlight: at the moment,
>>> more than 90% of reviews are AT&T sponsored (counting independents
>>> working for at&t. See also
>>> https://www.stackalytics.com/?module=openstack-helm-group). That's very
>>> high.
>>>
>>> I believe extending the core team geographically/with different
>>> companies is a solution for the listed pain points.
>>
>>
>> I really would like to have that as well, however, efforts and time to become a candidate with "good enough" history seems very difficult.
>> Matching the level (or amount of works) with what the current core reviewers does is not an easy thing to achieve.
>> Frankly speaking, motivating someone to put that much effort is also challenging, especially with their reluctance (hesitant?) to do so due to language and time barrier.
>>
>>
>>>
>>>
>>> > However, I sometimes feel that turnaround time for some of patch sets
>>> > are
>>> > really long. I would like to hear opinion from others and suggestions
>>> > on
>>> > how to improve this. It can be either/both something each patch set
>>> > owner
>>> > need to do more, or/and it could be something we as a openstack-helm
>>> > project can improve. For instance, it could be influenced by time
>>> > differences, lack of irc presence, or anything else. etc. I really
>>> > would
>>> > like to find out there are anything we can improve together.
>>>
>>> I had the same impression myself: the turnaround time is big for a
>>> deployment project.
>>>
>>> The problem is not simple, and here are a few explanations I could
>>> think of:
>>> 1) most core reviewers are from a single company, and emergencies in
>>> their company are most likely to get prioritized over the community
>>> work. That leaves some reviews pending.
>>> 2) most core reviewers are from the same timezone in US, which means,
>>> in the best case, an asian contributor will have to wait a full day
>>> before seeing his work merged. If a core reviewer doesn't review this
>>> on his day work due to an emergency, you're putting the turnaround to
>>> two days at best.
>>> 3) most core reviewers are working in the same location: it's maybe
>>> hard for them to scale the conversation from their internal habits to a
>>> community driven project. Communication is a very important part of a
>>> community, and if that doesn't work, it is _very_ concerning to me. We
>>> raised the points of lack of (IRC presence|reviews) in previous
>>> community meetings.
>>
>>
>> 2-1) other active developers are on the opposite side of the earth, which make more difficult to sync with core reviewers. No one wanted, but it somehow creates an invisible barrier.
>>
>> I do agree that "Communication" is a very important part of a community.
>> Language and time differences are adding more difficulties on this as well. I am trying my best to be a good liaison, but never enough.
>> There will be no clear solution. However, I will have a discussion again with team members to gather some ideas.
>>
>>>
>>>
>>> >
>>> > I would like to get any kind of advise on the following.
>>> > - sometimes, it is really difficult to get core reviewers' comments
>>> > or
>>> > reviews. I routinely put the list of patch sets on irc meeting
>>> > agenda,
>>> > however, there still be a long turnaround time between comments. As a
>>> > result, it usually takes a long time to process a patch set, does
>>> > sometimes
>>> > cause rebase as well.
>>>
>>> I thank our testing system auto rebases a lot :)
>>> The bigger problem is when you're working on something which eventually
>>> conflicts with some AT&T work that was prioritized internally.
>>>
>>> For that, I asked a clear list of what the priorities are.
>>> ( https://storyboard.openstack.org/#!/worklist/341 )
>>>
>>> Anything outside that should IMO raise a little flag in our heads :)
>>>
>>> But it's up to the core reviewers to work with this in focus, and to
>>> the PTL to give directions.
>>>
>>>
>>> > - Having said that, I would like to have any advise on what we need
>>> > to do
>>> > more, for instance, do we need to be in irc directly asking each
>>> > patch set
>>> > to core reviewers? do we need to put core reviewers' name when we
>>> > push
>>> > patch set? etc.
>>>
>>> I believe that we should leverage IRC more for reviews. We are doing it
>>> in OSA, and it works fine. Of course core developers have their habits
>>> and a review dashboard, but fast/emergency reviews need to be
>>> socialized to get prioritized. There are other attempts in the
>>> community (like have a review priority in gerrit), but I am not
>>> entirely sold on bringing a technical solution to something that should
>>> be solved with more communication.
>>>
>>> > - Some of patch sets are being reviewed and merged quickly, and some
>>> > of
>>> > patch sets are not. I would like to know what makes this difference
>>> > so that
>>> > I can tell my developers how to do better job writing and
>>> > communicating
>>> > patch sets.
>>> >
>>> > There are just some example patch sets currently under review stage.
>>> >
>>> > 1. https://review.openstack.org/#/c/603971/ >> this ps has been
>>> > discussed
>>> > for its contents and scope. Cloud you please add if there is anything
>>> > else
>>> > we need to do other than wrapping some of commit message?
>>> >
>>> > 2. https://review.openstack.org/#/c/633456/ >> this is simple fix.
>>> > how can
>>> > we make core reviewer notice this patch set so that they can quickly
>>> > view?
>>> >
>>> > 3. https://review.openstack.org/#/c/625803/ >> we have been getting
>>> > feedbacks and questions on this patch set, that has been good. but
>>> > round-trip time for the recent comments takes a week or more. because
>>> > of
>>> > that delay (?), the owner of this patch set needed to rebase this one
>>> > often. Will this kind of case be improved if author engages more on
>>> > irc
>>> > channel or via mailing list to get feedback rather than relying on
>>> > gerrit
>>> > reviews?
>>>
>>> To me, the last one is more controversial than others (I don't believe
>>> we should give the opportunity to do that myself until we've done a
>>> security impact analysis). This change is also bigger than others,
>>> which is harder to both write and review. As far as I know, there was
>>> no spec that preceeded this work, so we couldn't discuss the approach
>>> before the code was written.
>>>
>>> I don't mind not having specs for changes to be honest, but it makes
>>> sense to have one if the subject is more controversial/harder, because
>>> people will have a tendency to put hard job aside.
>>>
>>> This review is the typical review that needs to be discussed in the
>>> community meeting, advocating for or against it until a decision is
>>> taken (merge or abandon).
>>
>>
>> I do agree on your analysis on this one. but, One thing the author really wanted to have was feedback, that can be either negative or positive. it could be something to ask to abandon, or rewrite.
>> but lack of comments with a long turnaround time between comments (that means author waits days and weeks to see any additional comments) was the problem.
>> It felt like somewhat abandoned without any strong reason.
>>
>>
>>>
>>>
>>> >
>>> > Frankly speaking, I don't know if this is a real issue or just way it
>>> > is. I
>>> > just want to pass some of voice from our developers, and really would
>>> > like
>>> > to hear what others think and find a better way to communicate.
>>>
>>> It doesn't matter if "it's a real issue" or "just the way it is".
>>> If there is a feeling of burden/pain, we should tackle the issue.
>>>
>>> So, yes, it's very important to raise the issue you feel!
>>> If you don't do it, nothing will change, the morale of developers will
>>> fall, and the health of the project will suffer.
>>> Transparency is key here.
>>>
>>> Thanks for voicing your opinion.
>>>
>>> >
>>> >
>>> > Thanks you.
>>> >
>>> >
>>>
>>> I would say my key take-aways are:
>>> 1) We need to review more
>>> 2) We need to communicate/socialize more on patchsets and issues. Let's
>>> be more active on IRC outside meetings.
>>
>>
>> Just one small note here: developers in my team prefer email communication sometime, where they can have time to think how to write their opinion on English.
>>
>>>
>>> 3) The priority list need to be updated to be accurate. I am not sure
>>> this list is complete (there is no mention of docs image building
>>> there).
>>
>>
>> I really want this happen. Things are often suddenly showed up on patch set and merged.
>> It is a bit difficult to follow what is exactly happening on openstack-helm community. Of course, this required everyone's efforts.
>>
>>>
>>> 4) We need to extend the core team in different geographical regions
>>> and companies as soon as possible
>>>
>>> But of course it's only my analysis. I would be happy to see Pete
>>> answer here.
>>>
>>> Regards,
>>> Jeam-Philippe Evrard (evrardjp)
>>>
>>>
>>
>> A bit unrelated with topic, but I really want to say this.
>> I DO REALLY appreciate openstack-helm community's effort to accept non-English documents as official one. (although it is slowly progressing ^^)
>> I think this move is real diversity effort than any other move (recognizing there is a good value community need to bring in "as-is", even though that is non-English information)
>>
>> Cheers,
>>
>>
>> --
>> Jaesuk Ahn, Ph.D.
>> Software R&D Center, SK Telecom
>
>
>
> --
> Jaesuk Ahn, Ph.D.
> Software R&D Center, SK Telecom
More information about the openstack-discuss
mailing list