cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Edison Su <Edison...@citrix.com>
Subject RE: [DISCUSS] Do we need code review process for code changes related to storage subsystem?
Date Fri, 21 Jun 2013 20:40:57 GMT


> -----Original Message-----
> From: Chip Childers [mailto:chip.childers@sungard.com]
> Sent: Friday, June 21, 2013 1:22 PM
> To: <dev@cloudstack.apache.org>
> Subject: Re: [DISCUSS] Do we need code review process for code changes
> related to storage subsystem?
> 
> On Jun 21, 2013, at 4:18 PM, Edison Su <Edison.su@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.

Because, there is no other body call for reviewing the change before. If we can make it as
a standard process for all the changes related to interfaces, db changes,  on cloudstack code,
and there are people like to review the changes, then let's do it.

> 
> >
> >> -----Original Message-----
> >> From: John Burwell [mailto:jburwell@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.su@citrix.com> wrote:
> >>
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: John Burwell [mailto:jburwell@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.su@citrix.com>
wrote:
> >>>>
> >>>>>
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Chip Childers [mailto:chip.childers@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
> >
> >

Mime
View raw message