You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Edison Su <Ed...@citrix.com> on 2013/06/20 19:59:01 UTC
[DISCUSS] Do we need code review process for code changes related
to storage subsystem?
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.
Re: [DISCUSS] Do we need code review process for code changes related
to storage subsystem?
Posted by Mike Tutkowski <mi...@solidfire.com>.
Just as an FYI, the code I sent off to John Burwell for review "accepts"
the fact that we require a hypervisor type when creating primary storage
now and expects the admin to pass in the type of "Any".
I then made a small change elsewhere in the codebase so this would work in
my situation.
It might be late in the game to change this hypervisor field being
required, but I just wanted to let you guys know that I've written code for
my situation to get around it.
Thanks!
On Fri, Jun 21, 2013 at 12:15 PM, Edison Su <Ed...@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
>
--
*Mike Tutkowski*
*Senior CloudStack Developer, SolidFire Inc.*
e: mike.tutkowski@solidfire.com
o: 303.746.7302
Advancing the way the world uses the
cloud<http://solidfire.com/solution/overview/?video=play>
*™*
RE: [DISCUSS] Do we need code review process for code changes
related to storage subsystem?
Posted by Edison Su <Ed...@citrix.com>.
> -----Original Message-----
> From: Chip Childers [mailto:chip.childers@sungard.com]
> Sent: Friday, June 21, 2013 1:22 PM
> To: <de...@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 <Ed...@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 <Ed...@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 <Ed...@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
> >
> >
Re: [DISCUSS] Do we need code review process for code changes related
to storage subsystem?
Posted by Chip Childers <ch...@sungard.com>.
On Jun 21, 2013, at 4:18 PM, Edison Su <Ed...@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: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 <Ed...@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 <Ed...@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
>
>
RE: [DISCUSS] Do we need code review process for code changes
related to storage subsystem?
Posted by Edison Su <Ed...@citrix.com>.
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:)
> -----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 <Ed...@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 <Ed...@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
> >
Re: [DISCUSS] Do we need code review process for code changes related to storage subsystem?
Posted by John Burwell <jb...@basho.com>.
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 <Ed...@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 <Ed...@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
>
Re: [DISCUSS] Do we need code review process for code changes
related to storage subsystem?
Posted by Prasanna Santhanam <ts...@apache.org>.
On Fri, Jun 21, 2013 at 07:53:21PM +0000, Edison Su wrote:
> > 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"?
Edison, I think the problem is easily solved if people learn to do
"rebase" instead of "merge". When we diverge into topic branches for
our features we end up in complete silos. If you do a regular rebase
of your topic branch you are aware of the changes happening on the
master branch. That precludes everyone from having to look at
review/merge requests.
Eg: If dev A is doing feature X that disrupts dev B doing feature Y
both in their own branches. If dev A brings in his feature first to
master and dev B rebases on top, B knows that his feature breaks when
he rebases against master above dev A's feature X. By doing a merge B
simply assumes his feature works alongside A's feature. At least then
B, even if he ignored the merge request from A, will make noise on the
list to fix it appropriately in collaboration with A.
Our proposals /specs already include all the db changes and api
changes. But like you said, not everyone is looking at them and
preemptively nipping the possiblity of such conflicts. So a more
pro-active process of keeping master as the one true state of the
project and working additively on top of each other will prevent these
frustrations.
What do you think?
--
Prasanna.,
------------------------
Powered by BigRock.com
RE: [DISCUSS] Do we need code review process for code changes
related to storage subsystem?
Posted by Edison Su <Ed...@citrix.com>.
> -----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 <Ed...@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
Re: [DISCUSS] Do we need code review process for code changes related to storage subsystem?
Posted by John Burwell <jb...@basho.com>.
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 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 <Ed...@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
RE: [DISCUSS] Do we need code review process for code changes
related to storage subsystem?
Posted by Edison Su <Ed...@citrix.com>.
> -----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
Re: [DISCUSS] Do we need code review process for code changes related
to storage subsystem?
Posted by John Burwell <jb...@basho.com>.
Prasanna,
I believe the problem Edison is trying to address is high churn and
rework incurred by long review cycles. I find it hard to review
interfaces in isolation without implementations depicting actual
usage. I would also like to see process ceremony kept to minimum to
reduce drag on the over project flow. To that end, I think the best
approach is to divide large enhancements into a batch of smaller
commits that can be reviewed throughout the release cycle. I think
this approach will reduce churn and rework while maintaing high impact
reviews.
Thanks,
-John
On Jun 21, 2013, at 12:31 AM, Prasanna Santhanam <ts...@apache.org> wrote:
> On Thu, Jun 20, 2013 at 08:41:56PM -0400, Chip Childers wrote:
>> 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.
>
> May be that everything interface related goes through
> review/discussion when touching the storage subsys? I don't mind that
> for other base components as well. But probably a good idea for
> storage since it's still nascent and evolving.
>
> --
> Prasanna.,
>
> ------------------------
> Powered by BigRock.com
>
Re: [DISCUSS] Do we need code review process for code changes
related to storage subsystem?
Posted by Prasanna Santhanam <ts...@apache.org>.
On Thu, Jun 20, 2013 at 08:41:56PM -0400, Chip Childers wrote:
> 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.
>
May be that everything interface related goes through
review/discussion when touching the storage subsys? I don't mind that
for other base components as well. But probably a good idea for
storage since it's still nascent and evolving.
--
Prasanna.,
------------------------
Powered by BigRock.com
Re: [DISCUSS] Do we need code review process for code changes
related to storage subsystem?
Posted by Chip Childers <ch...@sungard.com>.
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.
-chip
Re: [DISCUSS] Do we need code review process for code changes related
to storage subsystem?
Posted by Mike Tutkowski <mi...@solidfire.com>.
I agree.
I've had to make slight changes to the interface a storage plug-in
implements (around createAsync and deleteAsync), but that should be fine
since 4.2 is the first release to expose this interface.
On Thu, Jun 20, 2013 at 11:59 AM, Edison Su <Ed...@citrix.com> 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.
>
--
*Mike Tutkowski*
*Senior CloudStack Developer, SolidFire Inc.*
e: mike.tutkowski@solidfire.com
o: 303.746.7302
Advancing the way the world uses the
cloud<http://solidfire.com/solution/overview/?video=play>
*™*