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@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:
- 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.
- 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?
- 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?
- 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:
- We need to review more
- 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.
- 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.
- 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,