On Jun 21, 2013, at 4:18 PM, Edison Su <edison...@citrix.com> wrote: > How about, for all the interfaces, DB schema changes, related to storage > subsystem, need to send out a merge request and push the patches into review > board? It's not only for feature development, but also for bug fixes. > I am not sure how many people want to review the changes related to storage > subsystem, though. If only I am interested in it, then don't need to do that:)
I don't understand why storage is different from the rest of the code. > >> -----Original Message----- >> From: John Burwell [mailto:jburw...@basho.com] >> Sent: Friday, June 21, 2013 1:00 PM >> To: dev@cloudstack.apache.org >> Cc: 'Chip Childers' >> Subject: Re: [DISCUSS] Do we need code review process for code changes >> related to storage subsystem? >> >> Edison, >> >> The person pushing the merge request should highlight that it includes >> interface changes (regardless of whether or not it is a storage patch). I >> also >> think that we should be pushing all patches that rise to merge requests into >> Review Board to allow potential reviewers a place to cleanly communicate >> and discuss issues. >> >> Thanks, >> -John >> >> On Jun 21, 2013, at 3:53 PM, Edison Su <edison...@citrix.com> wrote: >> >>> >>> >>>> -----Original Message----- >>>> From: John Burwell [mailto:jburw...@basho.com] >>>> Sent: Friday, June 21, 2013 11:43 AM >>>> To: dev@cloudstack.apache.org >>>> Cc: 'Chip Childers' >>>> Subject: Re: [DISCUSS] Do we need code review process for code >>>> changes related to storage subsystem? >>>> >>>> Edison, >>>> >>>> My understanding of our process is that the merges of significant >>>> changes should be proposed to the mailing list with the "[MERGE]" tag >>>> and wait up to >>>> 72 hours for feedback. I consider interface changes to meet that >>>> criteria given the potential to break other folks work. It sounds >>>> like we had a change that inadvertently slipped through without >>>> notice to list. Going forward, I >>> >>> The problem of current merge request, is that, you don't know what kind >> of change the merge request did, unless you dig into the code. >>> Let's say, there is a merge request, the code touches both vm and storage >> code, then how do I know, by just taking look at the request, that, the >> storage part of code is been changed. >>> As there are lot of merge requests, a single person can't review all the >> merge requests, then likely, the change is slipped without the notice to >> other >> people who wants to review storage related code, even if the merge request >> is send out to the list. >>> >>> Maybe, we should ask for each merge request, need to add a list of files >> been changed: like the output of "git diff --stat"? >>> >>>> propose that we follow this process for significant patches and, >>>> additionally, push them to Review Board. As a matter of >>>> collaboration, it might also be a good idea to open a "[DISCUSS]" >>>> thread during the design/planning stages, but I don't think we need to >> require it. >>>> >>>> Do you think this approach will properly balance to our needs to >>>> coordinate/review work with maintaining a smooth flow? >>>> >>>> Thanks, >>>> -John >>>> >>>> >>>> On Jun 21, 2013, at 2:15 PM, Edison Su <edison...@citrix.com> wrote: >>>> >>>>> >>>>> >>>>>> -----Original Message----- >>>>>> From: Chip Childers [mailto:chip.child...@sungard.com] >>>>>> Sent: Thursday, June 20, 2013 5:42 PM >>>>>> To: dev@cloudstack.apache.org >>>>>> Cc: Edison Su >>>>>> Subject: Re: [DISCUSS] Do we need code review process for code >>>>>> changes related to storage subsystem? >>>>>> >>>>>> On Thu, Jun 20, 2013 at 05:59:01PM +0000, Edison Su wrote: >>>>>>> For interface/API changes, we'd better have a code review, as more >>>>>> storage vendors and more developers outside Citrix are contributing >>>>>> code to CloudStack storage subsystem. The code change should have >>>>>> less surprise for everybody who cares about storage subsystem. >>>>>> >>>>>> I'm not following what you are saying Edison. What are we not >>>>>> doing in this regard right now? I'm also not getting the "Citrix" >>>>>> point of >>>> reference here. >>>>> >>>>> We don't have a code review process for each commit currently, the >>>>> result >>>> of that, as the code evolving, people add more and more code, >>>> features, bug fixes etc, etc. Then maybe one month later, when you >>>> take a look at the code, which may be quite different than what you >>>> known about. So I want to add a code review process here, maybe start >> from storage subsystem at first. >>>>> The reason I add "Citrix" here, let's take a look at what happened >>>>> in the last >>>> month: >>>>> Mike, from SolidFire, is asking why there is a hypervisor field in >>>>> the storage >>>> pool, simply, the hypervisor field breaks his code. >>>>> And I don't understand why there is a column, called >>>>> dynamicallyScalable, >>>> in vm_template table. >>>>> I think you will understand my problem here... >>>>> >>>>> >>>>> >>>>>> >>>>>> -chip > >
