You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Darren Shepherd <da...@gmail.com> on 2013/10/23 21:52:21 UTC
StrategyPriority changes w/ Spring Changes
Chris, Edison,
You guys just committed 'Support Revert VM Disk from Snapshot.' At
the same time I was merging both my txn-refactor and
spring-modularization branches. They are really tricky merges and
each time I have to rebase it takes awhile to figure out. Anyhow,
your change + my changes breaks master. So I quickly rebased rb14823
and committed to master. rb14823 is the patch that makes the Storage
Strategies work with my spring work plus clean up some things.
Additionally I found out you can't inject List<SnapshotStrategy> to
the Snapshot object, so we really have to go with my change to
centralize the ownership of the strategies to a single class.
Can you please pull master and revalidate that I didn't break
anything, if its not too much of a pain.
Thanks,
Darren
Re: StrategyPriority changes w/ Spring Changes
Posted by Darren Shepherd <da...@gmail.com>.
I can fix it, let me see what I broke.
Darren
> On Oct 23, 2013, at 1:03 PM, "SuichII, Christopher" <Ch...@netapp.com> wrote:
>
> It looks like the changes from us didn’t make it through your merge at all. I should have been more clear in my review - there are logic changes that needed to be carried from my changes into yours, not simply a rebase. I will work on those changes and try to get a patch up shortly.
>
> --
> Chris Suich
> chris.suich@netapp.com
> NetApp Software Engineer
> Data Center Platforms – Cloud Solutions
> Citrix, Cisco & Red Hat
>
>> On Oct 23, 2013, at 3:52 PM, Darren Shepherd <da...@gmail.com> wrote:
>>
>> Chris, Edison,
>>
>> You guys just committed 'Support Revert VM Disk from Snapshot.' At
>> the same time I was merging both my txn-refactor and
>> spring-modularization branches. They are really tricky merges and
>> each time I have to rebase it takes awhile to figure out. Anyhow,
>> your change + my changes breaks master. So I quickly rebased rb14823
>> and committed to master. rb14823 is the patch that makes the Storage
>> Strategies work with my spring work plus clean up some things.
>> Additionally I found out you can't inject List<SnapshotStrategy> to
>> the Snapshot object, so we really have to go with my change to
>> centralize the ownership of the strategies to a single class.
>>
>> Can you please pull master and revalidate that I didn't break
>> anything, if its not too much of a pain.
>>
>> Thanks,
>> Darren
>
Re: StrategyPriority changes w/ Spring Changes
Posted by "SuichII, Christopher" <Ch...@netapp.com>.
It looks like the changes from us didn’t make it through your merge at all. I should have been more clear in my review - there are logic changes that needed to be carried from my changes into yours, not simply a rebase. I will work on those changes and try to get a patch up shortly.
--
Chris Suich
chris.suich@netapp.com
NetApp Software Engineer
Data Center Platforms – Cloud Solutions
Citrix, Cisco & Red Hat
On Oct 23, 2013, at 3:52 PM, Darren Shepherd <da...@gmail.com> wrote:
> Chris, Edison,
>
> You guys just committed 'Support Revert VM Disk from Snapshot.' At
> the same time I was merging both my txn-refactor and
> spring-modularization branches. They are really tricky merges and
> each time I have to rebase it takes awhile to figure out. Anyhow,
> your change + my changes breaks master. So I quickly rebased rb14823
> and committed to master. rb14823 is the patch that makes the Storage
> Strategies work with my spring work plus clean up some things.
> Additionally I found out you can't inject List<SnapshotStrategy> to
> the Snapshot object, so we really have to go with my change to
> centralize the ownership of the strategies to a single class.
>
> Can you please pull master and revalidate that I didn't break
> anything, if its not too much of a pain.
>
> Thanks,
> Darren
Re: StrategyPriority changes w/ Spring Changes
Posted by Darren Shepherd <da...@gmail.com>.
Updated f3e968b9830a667cb3b55477f1fe8259e56ceffb
Darren
On Wed, Oct 23, 2013 at 4:23 PM, Darren Shepherd
<da...@gmail.com> wrote:
> Frankly, I don't really care, I'll just change it to how it was
> before. I'll remove the get methods that return a collection.
>
> Darren
>
> On Wed, Oct 23, 2013 at 4:12 PM, SuichII, Christopher
> <Ch...@netapp.com> wrote:
>> If there are arguments against it, then lets keep the discussion going. I’m fine with sorting as well - it was requested by someone else to optimize this. However, just to play devil’s advocate: where would you need a sorted list of strategies rather than just needing the best fit?
>>
>> --
>> Chris Suich
>> chris.suich@netapp.com
>> NetApp Software Engineer
>> Data Center Platforms – Cloud Solutions
>> Citrix, Cisco & Red Hat
>>
>> On Oct 23, 2013, at 6:39 PM, Darren Shepherd <da...@gmail.com> wrote:
>>
>>> So your asking to not use the sorting logic and instead do the style of
>>>
>>> Priority highestPriority = Priority.CANT_HANDLE;
>>> Priority priority = strategy.canHandle(...);
>>> if (priority.ordinal() > highestPriority.ordinal()) {
>>> highestPriority = priority;
>>> strategyToUse = strategy;
>>> }
>>>
>>> The problem I have with the above style is that its never possible to
>>> get a sorted collection of strategies. That logic just allow you to
>>> get the best match. If you look at
>>> org.apache.cloudstack.engine.subsystem.api.storage.StorageStrategyFactory
>>> there are methods to get the first item and a collection. In terms of
>>> efficiently, it doesn't matter, were really optimizing a couple of CPU
>>> cycles.
>>>
>>> Darren
>>>
>>> On Wed, Oct 23, 2013 at 3:30 PM, SuichII, Christopher
>>> <Ch...@netapp.com> wrote:
>>>> Take a look at revision 3 of my changes here:
>>>> https://reviews.apache.org/r/14522/diff/3/#index_header
>>>>
>>>> The changes I made were due to discussion in the reviews. It should be
>>>> simpler, cleaner and more efficient logic than using comparators.
>>>>
>>>> --
>>>> Chris Suich
>>>> chris.suich@netapp.com
>>>> NetApp Software Engineer
>>>> Data Center Platforms – Cloud Solutions
>>>> Citrix, Cisco & Red Hat
>>>>
>>>> On Oct 23, 2013, at 6:25 PM, Darren Shepherd <da...@gmail.com>
>>>> wrote:
>>>>
>>>> Sorry, I don't follow your question. I have time today. What would
>>>> you like me to do? As it stands right now, what is on master, I'm not
>>>> aware of any issues.
>>>>
>>>> Darren
>>>>
>>>> On Wed, Oct 23, 2013 at 3:22 PM, SuichII, Christopher
>>>> <Ch...@netapp.com> wrote:
>>>>
>>>> Darren,
>>>>
>>>> Would you be able to look into copy the logic back into your refactoring
>>>> today or tomorrow? If not, I may be able to in the morning.
>>>>
>>>> -Chris
>>>> --
>>>> Chris Suich
>>>> chris.suich@netapp.com
>>>> NetApp Software Engineer
>>>> Data Center Platforms – Cloud Solutions
>>>> Citrix, Cisco & Red Hat
>>>>
>>>> On Oct 23, 2013, at 5:56 PM, SuichII, Christopher <Ch...@netapp.com>
>>>> wrote:
>>>>
>>>> My understanding is that it is still a work in progress to get those test
>>>> back running. Is this correct, Edison?
>>>>
>>>> --
>>>> Chris Suich
>>>> chris.suich@netapp.com
>>>> NetApp Software Engineer
>>>> Data Center Platforms – Cloud Solutions
>>>> Citrix, Cisco & Red Hat
>>>>
>>>> On Oct 23, 2013, at 5:48 PM, Darren Shepherd <da...@gmail.com>
>>>> wrote:
>>>>
>>>> I fixed all the compilation errors in engine/storage/integration-test.
>>>> I don't know how to run those test though, so I can't validate the
>>>> changes.
>>>>
>>>> Darren
>>>>
>>>> On Wed, Oct 23, 2013 at 1:53 PM, SuichII, Christopher
>>>> <Ch...@netapp.com> wrote:
>>>>
>>>> Yep. I’m running on a clean master.
>>>>
>>>> --
>>>> Chris Suich
>>>> chris.suich@netapp.com
>>>> NetApp Software Engineer
>>>> Data Center Platforms – Cloud Solutions
>>>> Citrix, Cisco & Red Hat
>>>>
>>>> On Oct 23, 2013, at 4:30 PM, Darren Shepherd <da...@gmail.com>
>>>> wrote:
>>>>
>>>> Okay let me look at that. Are you 100% sure your looking at a clean version
>>>> of master?
>>>>
>>>> Darren
>>>>
>>>> On Oct 23, 2013, at 1:17 PM, "SuichII, Christopher" <Ch...@netapp.com>
>>>> wrote:
>>>>
>>>> Er, sorry. That was poorly worded on my part. Some classes, like
>>>> SnapshotTest.java and all the storage providers, did not get updated
>>>> references to your refactoring. They still reference
>>>> StrategyPriority.pickStrategy(), etc. Additionally, I changed the
>>>> pickStrategy() logic from using a comparator to looping over the list once
>>>> keeping a reference to the best result. This logic was lost in the merge.
>>>>
>>>> --
>>>> Chris Suich
>>>> chris.suich@netapp.com
>>>> NetApp Software Engineer
>>>> Data Center Platforms – Cloud Solutions
>>>> Citrix, Cisco & Red Hat
>>>>
>>>> On Oct 23, 2013, at 4:13 PM, Darren Shepherd <da...@gmail.com>
>>>> wrote:
>>>>
>>>> The transaction API was changed in the merge. I could have maybe
>>>> missed updating a class. Let me check. When you said "It looks like
>>>> the changes from us didn’t make it through your merge at all," can you
>>>> point to something specific that got lost?
>>>>
>>>> Darren
>>>>
>>>> On Wed, Oct 23, 2013 at 1:05 PM, SuichII, Christopher
>>>> <Ch...@netapp.com> wrote:
>>>>
>>>> And it looks like some of your changes may have not merged correctly. I’m
>>>> getting compile errors like:
>>>>
>>>> The method close() is undefined for the type Transaction
>>>>
>>>> This shouldn’t have come from our merge.
>>>>
>>>> --
>>>> Chris Suich
>>>> chris.suich@netapp.com
>>>> NetApp Software Engineer
>>>> Data Center Platforms – Cloud Solutions
>>>> Citrix, Cisco & Red Hat
>>>>
>>>> On Oct 23, 2013, at 3:52 PM, Darren Shepherd <da...@gmail.com>
>>>> wrote:
>>>>
>>>> Chris, Edison,
>>>>
>>>> You guys just committed 'Support Revert VM Disk from Snapshot.' At
>>>> the same time I was merging both my txn-refactor and
>>>> spring-modularization branches. They are really tricky merges and
>>>> each time I have to rebase it takes awhile to figure out. Anyhow,
>>>> your change + my changes breaks master. So I quickly rebased rb14823
>>>> and committed to master. rb14823 is the patch that makes the Storage
>>>> Strategies work with my spring work plus clean up some things.
>>>> Additionally I found out you can't inject List<SnapshotStrategy> to
>>>> the Snapshot object, so we really have to go with my change to
>>>> centralize the ownership of the strategies to a single class.
>>>>
>>>> Can you please pull master and revalidate that I didn't break
>>>> anything, if its not too much of a pain.
>>>>
>>>> Thanks,
>>>> Darren
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>
Re: StrategyPriority changes w/ Spring Changes
Posted by Darren Shepherd <da...@gmail.com>.
Frankly, I don't really care, I'll just change it to how it was
before. I'll remove the get methods that return a collection.
Darren
On Wed, Oct 23, 2013 at 4:12 PM, SuichII, Christopher
<Ch...@netapp.com> wrote:
> If there are arguments against it, then lets keep the discussion going. I’m fine with sorting as well - it was requested by someone else to optimize this. However, just to play devil’s advocate: where would you need a sorted list of strategies rather than just needing the best fit?
>
> --
> Chris Suich
> chris.suich@netapp.com
> NetApp Software Engineer
> Data Center Platforms – Cloud Solutions
> Citrix, Cisco & Red Hat
>
> On Oct 23, 2013, at 6:39 PM, Darren Shepherd <da...@gmail.com> wrote:
>
>> So your asking to not use the sorting logic and instead do the style of
>>
>> Priority highestPriority = Priority.CANT_HANDLE;
>> Priority priority = strategy.canHandle(...);
>> if (priority.ordinal() > highestPriority.ordinal()) {
>> highestPriority = priority;
>> strategyToUse = strategy;
>> }
>>
>> The problem I have with the above style is that its never possible to
>> get a sorted collection of strategies. That logic just allow you to
>> get the best match. If you look at
>> org.apache.cloudstack.engine.subsystem.api.storage.StorageStrategyFactory
>> there are methods to get the first item and a collection. In terms of
>> efficiently, it doesn't matter, were really optimizing a couple of CPU
>> cycles.
>>
>> Darren
>>
>> On Wed, Oct 23, 2013 at 3:30 PM, SuichII, Christopher
>> <Ch...@netapp.com> wrote:
>>> Take a look at revision 3 of my changes here:
>>> https://reviews.apache.org/r/14522/diff/3/#index_header
>>>
>>> The changes I made were due to discussion in the reviews. It should be
>>> simpler, cleaner and more efficient logic than using comparators.
>>>
>>> --
>>> Chris Suich
>>> chris.suich@netapp.com
>>> NetApp Software Engineer
>>> Data Center Platforms – Cloud Solutions
>>> Citrix, Cisco & Red Hat
>>>
>>> On Oct 23, 2013, at 6:25 PM, Darren Shepherd <da...@gmail.com>
>>> wrote:
>>>
>>> Sorry, I don't follow your question. I have time today. What would
>>> you like me to do? As it stands right now, what is on master, I'm not
>>> aware of any issues.
>>>
>>> Darren
>>>
>>> On Wed, Oct 23, 2013 at 3:22 PM, SuichII, Christopher
>>> <Ch...@netapp.com> wrote:
>>>
>>> Darren,
>>>
>>> Would you be able to look into copy the logic back into your refactoring
>>> today or tomorrow? If not, I may be able to in the morning.
>>>
>>> -Chris
>>> --
>>> Chris Suich
>>> chris.suich@netapp.com
>>> NetApp Software Engineer
>>> Data Center Platforms – Cloud Solutions
>>> Citrix, Cisco & Red Hat
>>>
>>> On Oct 23, 2013, at 5:56 PM, SuichII, Christopher <Ch...@netapp.com>
>>> wrote:
>>>
>>> My understanding is that it is still a work in progress to get those test
>>> back running. Is this correct, Edison?
>>>
>>> --
>>> Chris Suich
>>> chris.suich@netapp.com
>>> NetApp Software Engineer
>>> Data Center Platforms – Cloud Solutions
>>> Citrix, Cisco & Red Hat
>>>
>>> On Oct 23, 2013, at 5:48 PM, Darren Shepherd <da...@gmail.com>
>>> wrote:
>>>
>>> I fixed all the compilation errors in engine/storage/integration-test.
>>> I don't know how to run those test though, so I can't validate the
>>> changes.
>>>
>>> Darren
>>>
>>> On Wed, Oct 23, 2013 at 1:53 PM, SuichII, Christopher
>>> <Ch...@netapp.com> wrote:
>>>
>>> Yep. I’m running on a clean master.
>>>
>>> --
>>> Chris Suich
>>> chris.suich@netapp.com
>>> NetApp Software Engineer
>>> Data Center Platforms – Cloud Solutions
>>> Citrix, Cisco & Red Hat
>>>
>>> On Oct 23, 2013, at 4:30 PM, Darren Shepherd <da...@gmail.com>
>>> wrote:
>>>
>>> Okay let me look at that. Are you 100% sure your looking at a clean version
>>> of master?
>>>
>>> Darren
>>>
>>> On Oct 23, 2013, at 1:17 PM, "SuichII, Christopher" <Ch...@netapp.com>
>>> wrote:
>>>
>>> Er, sorry. That was poorly worded on my part. Some classes, like
>>> SnapshotTest.java and all the storage providers, did not get updated
>>> references to your refactoring. They still reference
>>> StrategyPriority.pickStrategy(), etc. Additionally, I changed the
>>> pickStrategy() logic from using a comparator to looping over the list once
>>> keeping a reference to the best result. This logic was lost in the merge.
>>>
>>> --
>>> Chris Suich
>>> chris.suich@netapp.com
>>> NetApp Software Engineer
>>> Data Center Platforms – Cloud Solutions
>>> Citrix, Cisco & Red Hat
>>>
>>> On Oct 23, 2013, at 4:13 PM, Darren Shepherd <da...@gmail.com>
>>> wrote:
>>>
>>> The transaction API was changed in the merge. I could have maybe
>>> missed updating a class. Let me check. When you said "It looks like
>>> the changes from us didn’t make it through your merge at all," can you
>>> point to something specific that got lost?
>>>
>>> Darren
>>>
>>> On Wed, Oct 23, 2013 at 1:05 PM, SuichII, Christopher
>>> <Ch...@netapp.com> wrote:
>>>
>>> And it looks like some of your changes may have not merged correctly. I’m
>>> getting compile errors like:
>>>
>>> The method close() is undefined for the type Transaction
>>>
>>> This shouldn’t have come from our merge.
>>>
>>> --
>>> Chris Suich
>>> chris.suich@netapp.com
>>> NetApp Software Engineer
>>> Data Center Platforms – Cloud Solutions
>>> Citrix, Cisco & Red Hat
>>>
>>> On Oct 23, 2013, at 3:52 PM, Darren Shepherd <da...@gmail.com>
>>> wrote:
>>>
>>> Chris, Edison,
>>>
>>> You guys just committed 'Support Revert VM Disk from Snapshot.' At
>>> the same time I was merging both my txn-refactor and
>>> spring-modularization branches. They are really tricky merges and
>>> each time I have to rebase it takes awhile to figure out. Anyhow,
>>> your change + my changes breaks master. So I quickly rebased rb14823
>>> and committed to master. rb14823 is the patch that makes the Storage
>>> Strategies work with my spring work plus clean up some things.
>>> Additionally I found out you can't inject List<SnapshotStrategy> to
>>> the Snapshot object, so we really have to go with my change to
>>> centralize the ownership of the strategies to a single class.
>>>
>>> Can you please pull master and revalidate that I didn't break
>>> anything, if its not too much of a pain.
>>>
>>> Thanks,
>>> Darren
>>>
>>>
>>>
>>>
>>>
>>>
>
Re: StrategyPriority changes w/ Spring Changes
Posted by "SuichII, Christopher" <Ch...@netapp.com>.
If there are arguments against it, then lets keep the discussion going. I’m fine with sorting as well - it was requested by someone else to optimize this. However, just to play devil’s advocate: where would you need a sorted list of strategies rather than just needing the best fit?
--
Chris Suich
chris.suich@netapp.com
NetApp Software Engineer
Data Center Platforms – Cloud Solutions
Citrix, Cisco & Red Hat
On Oct 23, 2013, at 6:39 PM, Darren Shepherd <da...@gmail.com> wrote:
> So your asking to not use the sorting logic and instead do the style of
>
> Priority highestPriority = Priority.CANT_HANDLE;
> Priority priority = strategy.canHandle(...);
> if (priority.ordinal() > highestPriority.ordinal()) {
> highestPriority = priority;
> strategyToUse = strategy;
> }
>
> The problem I have with the above style is that its never possible to
> get a sorted collection of strategies. That logic just allow you to
> get the best match. If you look at
> org.apache.cloudstack.engine.subsystem.api.storage.StorageStrategyFactory
> there are methods to get the first item and a collection. In terms of
> efficiently, it doesn't matter, were really optimizing a couple of CPU
> cycles.
>
> Darren
>
> On Wed, Oct 23, 2013 at 3:30 PM, SuichII, Christopher
> <Ch...@netapp.com> wrote:
>> Take a look at revision 3 of my changes here:
>> https://reviews.apache.org/r/14522/diff/3/#index_header
>>
>> The changes I made were due to discussion in the reviews. It should be
>> simpler, cleaner and more efficient logic than using comparators.
>>
>> --
>> Chris Suich
>> chris.suich@netapp.com
>> NetApp Software Engineer
>> Data Center Platforms – Cloud Solutions
>> Citrix, Cisco & Red Hat
>>
>> On Oct 23, 2013, at 6:25 PM, Darren Shepherd <da...@gmail.com>
>> wrote:
>>
>> Sorry, I don't follow your question. I have time today. What would
>> you like me to do? As it stands right now, what is on master, I'm not
>> aware of any issues.
>>
>> Darren
>>
>> On Wed, Oct 23, 2013 at 3:22 PM, SuichII, Christopher
>> <Ch...@netapp.com> wrote:
>>
>> Darren,
>>
>> Would you be able to look into copy the logic back into your refactoring
>> today or tomorrow? If not, I may be able to in the morning.
>>
>> -Chris
>> --
>> Chris Suich
>> chris.suich@netapp.com
>> NetApp Software Engineer
>> Data Center Platforms – Cloud Solutions
>> Citrix, Cisco & Red Hat
>>
>> On Oct 23, 2013, at 5:56 PM, SuichII, Christopher <Ch...@netapp.com>
>> wrote:
>>
>> My understanding is that it is still a work in progress to get those test
>> back running. Is this correct, Edison?
>>
>> --
>> Chris Suich
>> chris.suich@netapp.com
>> NetApp Software Engineer
>> Data Center Platforms – Cloud Solutions
>> Citrix, Cisco & Red Hat
>>
>> On Oct 23, 2013, at 5:48 PM, Darren Shepherd <da...@gmail.com>
>> wrote:
>>
>> I fixed all the compilation errors in engine/storage/integration-test.
>> I don't know how to run those test though, so I can't validate the
>> changes.
>>
>> Darren
>>
>> On Wed, Oct 23, 2013 at 1:53 PM, SuichII, Christopher
>> <Ch...@netapp.com> wrote:
>>
>> Yep. I’m running on a clean master.
>>
>> --
>> Chris Suich
>> chris.suich@netapp.com
>> NetApp Software Engineer
>> Data Center Platforms – Cloud Solutions
>> Citrix, Cisco & Red Hat
>>
>> On Oct 23, 2013, at 4:30 PM, Darren Shepherd <da...@gmail.com>
>> wrote:
>>
>> Okay let me look at that. Are you 100% sure your looking at a clean version
>> of master?
>>
>> Darren
>>
>> On Oct 23, 2013, at 1:17 PM, "SuichII, Christopher" <Ch...@netapp.com>
>> wrote:
>>
>> Er, sorry. That was poorly worded on my part. Some classes, like
>> SnapshotTest.java and all the storage providers, did not get updated
>> references to your refactoring. They still reference
>> StrategyPriority.pickStrategy(), etc. Additionally, I changed the
>> pickStrategy() logic from using a comparator to looping over the list once
>> keeping a reference to the best result. This logic was lost in the merge.
>>
>> --
>> Chris Suich
>> chris.suich@netapp.com
>> NetApp Software Engineer
>> Data Center Platforms – Cloud Solutions
>> Citrix, Cisco & Red Hat
>>
>> On Oct 23, 2013, at 4:13 PM, Darren Shepherd <da...@gmail.com>
>> wrote:
>>
>> The transaction API was changed in the merge. I could have maybe
>> missed updating a class. Let me check. When you said "It looks like
>> the changes from us didn’t make it through your merge at all," can you
>> point to something specific that got lost?
>>
>> Darren
>>
>> On Wed, Oct 23, 2013 at 1:05 PM, SuichII, Christopher
>> <Ch...@netapp.com> wrote:
>>
>> And it looks like some of your changes may have not merged correctly. I’m
>> getting compile errors like:
>>
>> The method close() is undefined for the type Transaction
>>
>> This shouldn’t have come from our merge.
>>
>> --
>> Chris Suich
>> chris.suich@netapp.com
>> NetApp Software Engineer
>> Data Center Platforms – Cloud Solutions
>> Citrix, Cisco & Red Hat
>>
>> On Oct 23, 2013, at 3:52 PM, Darren Shepherd <da...@gmail.com>
>> wrote:
>>
>> Chris, Edison,
>>
>> You guys just committed 'Support Revert VM Disk from Snapshot.' At
>> the same time I was merging both my txn-refactor and
>> spring-modularization branches. They are really tricky merges and
>> each time I have to rebase it takes awhile to figure out. Anyhow,
>> your change + my changes breaks master. So I quickly rebased rb14823
>> and committed to master. rb14823 is the patch that makes the Storage
>> Strategies work with my spring work plus clean up some things.
>> Additionally I found out you can't inject List<SnapshotStrategy> to
>> the Snapshot object, so we really have to go with my change to
>> centralize the ownership of the strategies to a single class.
>>
>> Can you please pull master and revalidate that I didn't break
>> anything, if its not too much of a pain.
>>
>> Thanks,
>> Darren
>>
>>
>>
>>
>>
>>
Re: StrategyPriority changes w/ Spring Changes
Posted by Darren Shepherd <da...@gmail.com>.
So your asking to not use the sorting logic and instead do the style of
Priority highestPriority = Priority.CANT_HANDLE;
Priority priority = strategy.canHandle(...);
if (priority.ordinal() > highestPriority.ordinal()) {
highestPriority = priority;
strategyToUse = strategy;
}
The problem I have with the above style is that its never possible to
get a sorted collection of strategies. That logic just allow you to
get the best match. If you look at
org.apache.cloudstack.engine.subsystem.api.storage.StorageStrategyFactory
there are methods to get the first item and a collection. In terms of
efficiently, it doesn't matter, were really optimizing a couple of CPU
cycles.
Darren
On Wed, Oct 23, 2013 at 3:30 PM, SuichII, Christopher
<Ch...@netapp.com> wrote:
> Take a look at revision 3 of my changes here:
> https://reviews.apache.org/r/14522/diff/3/#index_header
>
> The changes I made were due to discussion in the reviews. It should be
> simpler, cleaner and more efficient logic than using comparators.
>
> --
> Chris Suich
> chris.suich@netapp.com
> NetApp Software Engineer
> Data Center Platforms – Cloud Solutions
> Citrix, Cisco & Red Hat
>
> On Oct 23, 2013, at 6:25 PM, Darren Shepherd <da...@gmail.com>
> wrote:
>
> Sorry, I don't follow your question. I have time today. What would
> you like me to do? As it stands right now, what is on master, I'm not
> aware of any issues.
>
> Darren
>
> On Wed, Oct 23, 2013 at 3:22 PM, SuichII, Christopher
> <Ch...@netapp.com> wrote:
>
> Darren,
>
> Would you be able to look into copy the logic back into your refactoring
> today or tomorrow? If not, I may be able to in the morning.
>
> -Chris
> --
> Chris Suich
> chris.suich@netapp.com
> NetApp Software Engineer
> Data Center Platforms – Cloud Solutions
> Citrix, Cisco & Red Hat
>
> On Oct 23, 2013, at 5:56 PM, SuichII, Christopher <Ch...@netapp.com>
> wrote:
>
> My understanding is that it is still a work in progress to get those test
> back running. Is this correct, Edison?
>
> --
> Chris Suich
> chris.suich@netapp.com
> NetApp Software Engineer
> Data Center Platforms – Cloud Solutions
> Citrix, Cisco & Red Hat
>
> On Oct 23, 2013, at 5:48 PM, Darren Shepherd <da...@gmail.com>
> wrote:
>
> I fixed all the compilation errors in engine/storage/integration-test.
> I don't know how to run those test though, so I can't validate the
> changes.
>
> Darren
>
> On Wed, Oct 23, 2013 at 1:53 PM, SuichII, Christopher
> <Ch...@netapp.com> wrote:
>
> Yep. I’m running on a clean master.
>
> --
> Chris Suich
> chris.suich@netapp.com
> NetApp Software Engineer
> Data Center Platforms – Cloud Solutions
> Citrix, Cisco & Red Hat
>
> On Oct 23, 2013, at 4:30 PM, Darren Shepherd <da...@gmail.com>
> wrote:
>
> Okay let me look at that. Are you 100% sure your looking at a clean version
> of master?
>
> Darren
>
> On Oct 23, 2013, at 1:17 PM, "SuichII, Christopher" <Ch...@netapp.com>
> wrote:
>
> Er, sorry. That was poorly worded on my part. Some classes, like
> SnapshotTest.java and all the storage providers, did not get updated
> references to your refactoring. They still reference
> StrategyPriority.pickStrategy(), etc. Additionally, I changed the
> pickStrategy() logic from using a comparator to looping over the list once
> keeping a reference to the best result. This logic was lost in the merge.
>
> --
> Chris Suich
> chris.suich@netapp.com
> NetApp Software Engineer
> Data Center Platforms – Cloud Solutions
> Citrix, Cisco & Red Hat
>
> On Oct 23, 2013, at 4:13 PM, Darren Shepherd <da...@gmail.com>
> wrote:
>
> The transaction API was changed in the merge. I could have maybe
> missed updating a class. Let me check. When you said "It looks like
> the changes from us didn’t make it through your merge at all," can you
> point to something specific that got lost?
>
> Darren
>
> On Wed, Oct 23, 2013 at 1:05 PM, SuichII, Christopher
> <Ch...@netapp.com> wrote:
>
> And it looks like some of your changes may have not merged correctly. I’m
> getting compile errors like:
>
> The method close() is undefined for the type Transaction
>
> This shouldn’t have come from our merge.
>
> --
> Chris Suich
> chris.suich@netapp.com
> NetApp Software Engineer
> Data Center Platforms – Cloud Solutions
> Citrix, Cisco & Red Hat
>
> On Oct 23, 2013, at 3:52 PM, Darren Shepherd <da...@gmail.com>
> wrote:
>
> Chris, Edison,
>
> You guys just committed 'Support Revert VM Disk from Snapshot.' At
> the same time I was merging both my txn-refactor and
> spring-modularization branches. They are really tricky merges and
> each time I have to rebase it takes awhile to figure out. Anyhow,
> your change + my changes breaks master. So I quickly rebased rb14823
> and committed to master. rb14823 is the patch that makes the Storage
> Strategies work with my spring work plus clean up some things.
> Additionally I found out you can't inject List<SnapshotStrategy> to
> the Snapshot object, so we really have to go with my change to
> centralize the ownership of the strategies to a single class.
>
> Can you please pull master and revalidate that I didn't break
> anything, if its not too much of a pain.
>
> Thanks,
> Darren
>
>
>
>
>
>
Re: StrategyPriority changes w/ Spring Changes
Posted by "SuichII, Christopher" <Ch...@netapp.com>.
Take a look at revision 3 of my changes here: https://reviews.apache.org/r/14522/diff/3/#index_header
The changes I made were due to discussion in the reviews. It should be simpler, cleaner and more efficient logic than using comparators.
--
Chris Suich
chris.suich@netapp.com<ma...@netapp.com>
NetApp Software Engineer
Data Center Platforms – Cloud Solutions
Citrix, Cisco & Red Hat
On Oct 23, 2013, at 6:25 PM, Darren Shepherd <da...@gmail.com>> wrote:
Sorry, I don't follow your question. I have time today. What would
you like me to do? As it stands right now, what is on master, I'm not
aware of any issues.
Darren
On Wed, Oct 23, 2013 at 3:22 PM, SuichII, Christopher
<Ch...@netapp.com>> wrote:
Darren,
Would you be able to look into copy the logic back into your refactoring today or tomorrow? If not, I may be able to in the morning.
-Chris
--
Chris Suich
chris.suich@netapp.com<ma...@netapp.com>
NetApp Software Engineer
Data Center Platforms – Cloud Solutions
Citrix, Cisco & Red Hat
On Oct 23, 2013, at 5:56 PM, SuichII, Christopher <Ch...@netapp.com> wrote:
My understanding is that it is still a work in progress to get those test back running. Is this correct, Edison?
--
Chris Suich
chris.suich@netapp.com
NetApp Software Engineer
Data Center Platforms – Cloud Solutions
Citrix, Cisco & Red Hat
On Oct 23, 2013, at 5:48 PM, Darren Shepherd <da...@gmail.com> wrote:
I fixed all the compilation errors in engine/storage/integration-test.
I don't know how to run those test though, so I can't validate the
changes.
Darren
On Wed, Oct 23, 2013 at 1:53 PM, SuichII, Christopher
<Ch...@netapp.com> wrote:
Yep. I’m running on a clean master.
--
Chris Suich
chris.suich@netapp.com
NetApp Software Engineer
Data Center Platforms – Cloud Solutions
Citrix, Cisco & Red Hat
On Oct 23, 2013, at 4:30 PM, Darren Shepherd <da...@gmail.com> wrote:
Okay let me look at that. Are you 100% sure your looking at a clean version of master?
Darren
On Oct 23, 2013, at 1:17 PM, "SuichII, Christopher" <Ch...@netapp.com> wrote:
Er, sorry. That was poorly worded on my part. Some classes, like SnapshotTest.java and all the storage providers, did not get updated references to your refactoring. They still reference StrategyPriority.pickStrategy(), etc. Additionally, I changed the pickStrategy() logic from using a comparator to looping over the list once keeping a reference to the best result. This logic was lost in the merge.
--
Chris Suich
chris.suich@netapp.com
NetApp Software Engineer
Data Center Platforms – Cloud Solutions
Citrix, Cisco & Red Hat
On Oct 23, 2013, at 4:13 PM, Darren Shepherd <da...@gmail.com> wrote:
The transaction API was changed in the merge. I could have maybe
missed updating a class. Let me check. When you said "It looks like
the changes from us didn’t make it through your merge at all," can you
point to something specific that got lost?
Darren
On Wed, Oct 23, 2013 at 1:05 PM, SuichII, Christopher
<Ch...@netapp.com> wrote:
And it looks like some of your changes may have not merged correctly. I’m getting compile errors like:
The method close() is undefined for the type Transaction
This shouldn’t have come from our merge.
--
Chris Suich
chris.suich@netapp.com
NetApp Software Engineer
Data Center Platforms – Cloud Solutions
Citrix, Cisco & Red Hat
On Oct 23, 2013, at 3:52 PM, Darren Shepherd <da...@gmail.com> wrote:
Chris, Edison,
You guys just committed 'Support Revert VM Disk from Snapshot.' At
the same time I was merging both my txn-refactor and
spring-modularization branches. They are really tricky merges and
each time I have to rebase it takes awhile to figure out. Anyhow,
your change + my changes breaks master. So I quickly rebased rb14823
and committed to master. rb14823 is the patch that makes the Storage
Strategies work with my spring work plus clean up some things.
Additionally I found out you can't inject List<SnapshotStrategy> to
the Snapshot object, so we really have to go with my change to
centralize the ownership of the strategies to a single class.
Can you please pull master and revalidate that I didn't break
anything, if its not too much of a pain.
Thanks,
Darren
Re: StrategyPriority changes w/ Spring Changes
Posted by Darren Shepherd <da...@gmail.com>.
Sorry, I don't follow your question. I have time today. What would
you like me to do? As it stands right now, what is on master, I'm not
aware of any issues.
Darren
On Wed, Oct 23, 2013 at 3:22 PM, SuichII, Christopher
<Ch...@netapp.com> wrote:
> Darren,
>
> Would you be able to look into copy the logic back into your refactoring today or tomorrow? If not, I may be able to in the morning.
>
> -Chris
> --
> Chris Suich
> chris.suich@netapp.com
> NetApp Software Engineer
> Data Center Platforms – Cloud Solutions
> Citrix, Cisco & Red Hat
>
> On Oct 23, 2013, at 5:56 PM, SuichII, Christopher <Ch...@netapp.com> wrote:
>
>> My understanding is that it is still a work in progress to get those test back running. Is this correct, Edison?
>>
>> --
>> Chris Suich
>> chris.suich@netapp.com
>> NetApp Software Engineer
>> Data Center Platforms – Cloud Solutions
>> Citrix, Cisco & Red Hat
>>
>> On Oct 23, 2013, at 5:48 PM, Darren Shepherd <da...@gmail.com> wrote:
>>
>>> I fixed all the compilation errors in engine/storage/integration-test.
>>> I don't know how to run those test though, so I can't validate the
>>> changes.
>>>
>>> Darren
>>>
>>> On Wed, Oct 23, 2013 at 1:53 PM, SuichII, Christopher
>>> <Ch...@netapp.com> wrote:
>>>> Yep. I’m running on a clean master.
>>>>
>>>> --
>>>> Chris Suich
>>>> chris.suich@netapp.com
>>>> NetApp Software Engineer
>>>> Data Center Platforms – Cloud Solutions
>>>> Citrix, Cisco & Red Hat
>>>>
>>>> On Oct 23, 2013, at 4:30 PM, Darren Shepherd <da...@gmail.com> wrote:
>>>>
>>>>> Okay let me look at that. Are you 100% sure your looking at a clean version of master?
>>>>>
>>>>> Darren
>>>>>
>>>>>> On Oct 23, 2013, at 1:17 PM, "SuichII, Christopher" <Ch...@netapp.com> wrote:
>>>>>>
>>>>>> Er, sorry. That was poorly worded on my part. Some classes, like SnapshotTest.java and all the storage providers, did not get updated references to your refactoring. They still reference StrategyPriority.pickStrategy(), etc. Additionally, I changed the pickStrategy() logic from using a comparator to looping over the list once keeping a reference to the best result. This logic was lost in the merge.
>>>>>>
>>>>>> --
>>>>>> Chris Suich
>>>>>> chris.suich@netapp.com
>>>>>> NetApp Software Engineer
>>>>>> Data Center Platforms – Cloud Solutions
>>>>>> Citrix, Cisco & Red Hat
>>>>>>
>>>>>>> On Oct 23, 2013, at 4:13 PM, Darren Shepherd <da...@gmail.com> wrote:
>>>>>>>
>>>>>>> The transaction API was changed in the merge. I could have maybe
>>>>>>> missed updating a class. Let me check. When you said "It looks like
>>>>>>> the changes from us didn’t make it through your merge at all," can you
>>>>>>> point to something specific that got lost?
>>>>>>>
>>>>>>> Darren
>>>>>>>
>>>>>>> On Wed, Oct 23, 2013 at 1:05 PM, SuichII, Christopher
>>>>>>> <Ch...@netapp.com> wrote:
>>>>>>>> And it looks like some of your changes may have not merged correctly. I’m getting compile errors like:
>>>>>>>>
>>>>>>>> The method close() is undefined for the type Transaction
>>>>>>>>
>>>>>>>> This shouldn’t have come from our merge.
>>>>>>>>
>>>>>>>> --
>>>>>>>> Chris Suich
>>>>>>>> chris.suich@netapp.com
>>>>>>>> NetApp Software Engineer
>>>>>>>> Data Center Platforms – Cloud Solutions
>>>>>>>> Citrix, Cisco & Red Hat
>>>>>>>>
>>>>>>>>> On Oct 23, 2013, at 3:52 PM, Darren Shepherd <da...@gmail.com> wrote:
>>>>>>>>>
>>>>>>>>> Chris, Edison,
>>>>>>>>>
>>>>>>>>> You guys just committed 'Support Revert VM Disk from Snapshot.' At
>>>>>>>>> the same time I was merging both my txn-refactor and
>>>>>>>>> spring-modularization branches. They are really tricky merges and
>>>>>>>>> each time I have to rebase it takes awhile to figure out. Anyhow,
>>>>>>>>> your change + my changes breaks master. So I quickly rebased rb14823
>>>>>>>>> and committed to master. rb14823 is the patch that makes the Storage
>>>>>>>>> Strategies work with my spring work plus clean up some things.
>>>>>>>>> Additionally I found out you can't inject List<SnapshotStrategy> to
>>>>>>>>> the Snapshot object, so we really have to go with my change to
>>>>>>>>> centralize the ownership of the strategies to a single class.
>>>>>>>>>
>>>>>>>>> Can you please pull master and revalidate that I didn't break
>>>>>>>>> anything, if its not too much of a pain.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Darren
>>>>>>
>>>>
>>
>
Re: StrategyPriority changes w/ Spring Changes
Posted by "SuichII, Christopher" <Ch...@netapp.com>.
Darren,
Would you be able to look into copy the logic back into your refactoring today or tomorrow? If not, I may be able to in the morning.
-Chris
--
Chris Suich
chris.suich@netapp.com
NetApp Software Engineer
Data Center Platforms – Cloud Solutions
Citrix, Cisco & Red Hat
On Oct 23, 2013, at 5:56 PM, SuichII, Christopher <Ch...@netapp.com> wrote:
> My understanding is that it is still a work in progress to get those test back running. Is this correct, Edison?
>
> --
> Chris Suich
> chris.suich@netapp.com
> NetApp Software Engineer
> Data Center Platforms – Cloud Solutions
> Citrix, Cisco & Red Hat
>
> On Oct 23, 2013, at 5:48 PM, Darren Shepherd <da...@gmail.com> wrote:
>
>> I fixed all the compilation errors in engine/storage/integration-test.
>> I don't know how to run those test though, so I can't validate the
>> changes.
>>
>> Darren
>>
>> On Wed, Oct 23, 2013 at 1:53 PM, SuichII, Christopher
>> <Ch...@netapp.com> wrote:
>>> Yep. I’m running on a clean master.
>>>
>>> --
>>> Chris Suich
>>> chris.suich@netapp.com
>>> NetApp Software Engineer
>>> Data Center Platforms – Cloud Solutions
>>> Citrix, Cisco & Red Hat
>>>
>>> On Oct 23, 2013, at 4:30 PM, Darren Shepherd <da...@gmail.com> wrote:
>>>
>>>> Okay let me look at that. Are you 100% sure your looking at a clean version of master?
>>>>
>>>> Darren
>>>>
>>>>> On Oct 23, 2013, at 1:17 PM, "SuichII, Christopher" <Ch...@netapp.com> wrote:
>>>>>
>>>>> Er, sorry. That was poorly worded on my part. Some classes, like SnapshotTest.java and all the storage providers, did not get updated references to your refactoring. They still reference StrategyPriority.pickStrategy(), etc. Additionally, I changed the pickStrategy() logic from using a comparator to looping over the list once keeping a reference to the best result. This logic was lost in the merge.
>>>>>
>>>>> --
>>>>> Chris Suich
>>>>> chris.suich@netapp.com
>>>>> NetApp Software Engineer
>>>>> Data Center Platforms – Cloud Solutions
>>>>> Citrix, Cisco & Red Hat
>>>>>
>>>>>> On Oct 23, 2013, at 4:13 PM, Darren Shepherd <da...@gmail.com> wrote:
>>>>>>
>>>>>> The transaction API was changed in the merge. I could have maybe
>>>>>> missed updating a class. Let me check. When you said "It looks like
>>>>>> the changes from us didn’t make it through your merge at all," can you
>>>>>> point to something specific that got lost?
>>>>>>
>>>>>> Darren
>>>>>>
>>>>>> On Wed, Oct 23, 2013 at 1:05 PM, SuichII, Christopher
>>>>>> <Ch...@netapp.com> wrote:
>>>>>>> And it looks like some of your changes may have not merged correctly. I’m getting compile errors like:
>>>>>>>
>>>>>>> The method close() is undefined for the type Transaction
>>>>>>>
>>>>>>> This shouldn’t have come from our merge.
>>>>>>>
>>>>>>> --
>>>>>>> Chris Suich
>>>>>>> chris.suich@netapp.com
>>>>>>> NetApp Software Engineer
>>>>>>> Data Center Platforms – Cloud Solutions
>>>>>>> Citrix, Cisco & Red Hat
>>>>>>>
>>>>>>>> On Oct 23, 2013, at 3:52 PM, Darren Shepherd <da...@gmail.com> wrote:
>>>>>>>>
>>>>>>>> Chris, Edison,
>>>>>>>>
>>>>>>>> You guys just committed 'Support Revert VM Disk from Snapshot.' At
>>>>>>>> the same time I was merging both my txn-refactor and
>>>>>>>> spring-modularization branches. They are really tricky merges and
>>>>>>>> each time I have to rebase it takes awhile to figure out. Anyhow,
>>>>>>>> your change + my changes breaks master. So I quickly rebased rb14823
>>>>>>>> and committed to master. rb14823 is the patch that makes the Storage
>>>>>>>> Strategies work with my spring work plus clean up some things.
>>>>>>>> Additionally I found out you can't inject List<SnapshotStrategy> to
>>>>>>>> the Snapshot object, so we really have to go with my change to
>>>>>>>> centralize the ownership of the strategies to a single class.
>>>>>>>>
>>>>>>>> Can you please pull master and revalidate that I didn't break
>>>>>>>> anything, if its not too much of a pain.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Darren
>>>>>
>>>
>
Re: StrategyPriority changes w/ Spring Changes
Posted by "SuichII, Christopher" <Ch...@netapp.com>.
My understanding is that it is still a work in progress to get those test back running. Is this correct, Edison?
--
Chris Suich
chris.suich@netapp.com
NetApp Software Engineer
Data Center Platforms – Cloud Solutions
Citrix, Cisco & Red Hat
On Oct 23, 2013, at 5:48 PM, Darren Shepherd <da...@gmail.com> wrote:
> I fixed all the compilation errors in engine/storage/integration-test.
> I don't know how to run those test though, so I can't validate the
> changes.
>
> Darren
>
> On Wed, Oct 23, 2013 at 1:53 PM, SuichII, Christopher
> <Ch...@netapp.com> wrote:
>> Yep. I’m running on a clean master.
>>
>> --
>> Chris Suich
>> chris.suich@netapp.com
>> NetApp Software Engineer
>> Data Center Platforms – Cloud Solutions
>> Citrix, Cisco & Red Hat
>>
>> On Oct 23, 2013, at 4:30 PM, Darren Shepherd <da...@gmail.com> wrote:
>>
>>> Okay let me look at that. Are you 100% sure your looking at a clean version of master?
>>>
>>> Darren
>>>
>>>> On Oct 23, 2013, at 1:17 PM, "SuichII, Christopher" <Ch...@netapp.com> wrote:
>>>>
>>>> Er, sorry. That was poorly worded on my part. Some classes, like SnapshotTest.java and all the storage providers, did not get updated references to your refactoring. They still reference StrategyPriority.pickStrategy(), etc. Additionally, I changed the pickStrategy() logic from using a comparator to looping over the list once keeping a reference to the best result. This logic was lost in the merge.
>>>>
>>>> --
>>>> Chris Suich
>>>> chris.suich@netapp.com
>>>> NetApp Software Engineer
>>>> Data Center Platforms – Cloud Solutions
>>>> Citrix, Cisco & Red Hat
>>>>
>>>>> On Oct 23, 2013, at 4:13 PM, Darren Shepherd <da...@gmail.com> wrote:
>>>>>
>>>>> The transaction API was changed in the merge. I could have maybe
>>>>> missed updating a class. Let me check. When you said "It looks like
>>>>> the changes from us didn’t make it through your merge at all," can you
>>>>> point to something specific that got lost?
>>>>>
>>>>> Darren
>>>>>
>>>>> On Wed, Oct 23, 2013 at 1:05 PM, SuichII, Christopher
>>>>> <Ch...@netapp.com> wrote:
>>>>>> And it looks like some of your changes may have not merged correctly. I’m getting compile errors like:
>>>>>>
>>>>>> The method close() is undefined for the type Transaction
>>>>>>
>>>>>> This shouldn’t have come from our merge.
>>>>>>
>>>>>> --
>>>>>> Chris Suich
>>>>>> chris.suich@netapp.com
>>>>>> NetApp Software Engineer
>>>>>> Data Center Platforms – Cloud Solutions
>>>>>> Citrix, Cisco & Red Hat
>>>>>>
>>>>>>> On Oct 23, 2013, at 3:52 PM, Darren Shepherd <da...@gmail.com> wrote:
>>>>>>>
>>>>>>> Chris, Edison,
>>>>>>>
>>>>>>> You guys just committed 'Support Revert VM Disk from Snapshot.' At
>>>>>>> the same time I was merging both my txn-refactor and
>>>>>>> spring-modularization branches. They are really tricky merges and
>>>>>>> each time I have to rebase it takes awhile to figure out. Anyhow,
>>>>>>> your change + my changes breaks master. So I quickly rebased rb14823
>>>>>>> and committed to master. rb14823 is the patch that makes the Storage
>>>>>>> Strategies work with my spring work plus clean up some things.
>>>>>>> Additionally I found out you can't inject List<SnapshotStrategy> to
>>>>>>> the Snapshot object, so we really have to go with my change to
>>>>>>> centralize the ownership of the strategies to a single class.
>>>>>>>
>>>>>>> Can you please pull master and revalidate that I didn't break
>>>>>>> anything, if its not too much of a pain.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Darren
>>>>
>>
Re: StrategyPriority changes w/ Spring Changes
Posted by Darren Shepherd <da...@gmail.com>.
I fixed all the compilation errors in engine/storage/integration-test.
I don't know how to run those test though, so I can't validate the
changes.
Darren
On Wed, Oct 23, 2013 at 1:53 PM, SuichII, Christopher
<Ch...@netapp.com> wrote:
> Yep. I’m running on a clean master.
>
> --
> Chris Suich
> chris.suich@netapp.com
> NetApp Software Engineer
> Data Center Platforms – Cloud Solutions
> Citrix, Cisco & Red Hat
>
> On Oct 23, 2013, at 4:30 PM, Darren Shepherd <da...@gmail.com> wrote:
>
>> Okay let me look at that. Are you 100% sure your looking at a clean version of master?
>>
>> Darren
>>
>>> On Oct 23, 2013, at 1:17 PM, "SuichII, Christopher" <Ch...@netapp.com> wrote:
>>>
>>> Er, sorry. That was poorly worded on my part. Some classes, like SnapshotTest.java and all the storage providers, did not get updated references to your refactoring. They still reference StrategyPriority.pickStrategy(), etc. Additionally, I changed the pickStrategy() logic from using a comparator to looping over the list once keeping a reference to the best result. This logic was lost in the merge.
>>>
>>> --
>>> Chris Suich
>>> chris.suich@netapp.com
>>> NetApp Software Engineer
>>> Data Center Platforms – Cloud Solutions
>>> Citrix, Cisco & Red Hat
>>>
>>>> On Oct 23, 2013, at 4:13 PM, Darren Shepherd <da...@gmail.com> wrote:
>>>>
>>>> The transaction API was changed in the merge. I could have maybe
>>>> missed updating a class. Let me check. When you said "It looks like
>>>> the changes from us didn’t make it through your merge at all," can you
>>>> point to something specific that got lost?
>>>>
>>>> Darren
>>>>
>>>> On Wed, Oct 23, 2013 at 1:05 PM, SuichII, Christopher
>>>> <Ch...@netapp.com> wrote:
>>>>> And it looks like some of your changes may have not merged correctly. I’m getting compile errors like:
>>>>>
>>>>> The method close() is undefined for the type Transaction
>>>>>
>>>>> This shouldn’t have come from our merge.
>>>>>
>>>>> --
>>>>> Chris Suich
>>>>> chris.suich@netapp.com
>>>>> NetApp Software Engineer
>>>>> Data Center Platforms – Cloud Solutions
>>>>> Citrix, Cisco & Red Hat
>>>>>
>>>>>> On Oct 23, 2013, at 3:52 PM, Darren Shepherd <da...@gmail.com> wrote:
>>>>>>
>>>>>> Chris, Edison,
>>>>>>
>>>>>> You guys just committed 'Support Revert VM Disk from Snapshot.' At
>>>>>> the same time I was merging both my txn-refactor and
>>>>>> spring-modularization branches. They are really tricky merges and
>>>>>> each time I have to rebase it takes awhile to figure out. Anyhow,
>>>>>> your change + my changes breaks master. So I quickly rebased rb14823
>>>>>> and committed to master. rb14823 is the patch that makes the Storage
>>>>>> Strategies work with my spring work plus clean up some things.
>>>>>> Additionally I found out you can't inject List<SnapshotStrategy> to
>>>>>> the Snapshot object, so we really have to go with my change to
>>>>>> centralize the ownership of the strategies to a single class.
>>>>>>
>>>>>> Can you please pull master and revalidate that I didn't break
>>>>>> anything, if its not too much of a pain.
>>>>>>
>>>>>> Thanks,
>>>>>> Darren
>>>
>
Re: StrategyPriority changes w/ Spring Changes
Posted by "SuichII, Christopher" <Ch...@netapp.com>.
Yep. I’m running on a clean master.
--
Chris Suich
chris.suich@netapp.com
NetApp Software Engineer
Data Center Platforms – Cloud Solutions
Citrix, Cisco & Red Hat
On Oct 23, 2013, at 4:30 PM, Darren Shepherd <da...@gmail.com> wrote:
> Okay let me look at that. Are you 100% sure your looking at a clean version of master?
>
> Darren
>
>> On Oct 23, 2013, at 1:17 PM, "SuichII, Christopher" <Ch...@netapp.com> wrote:
>>
>> Er, sorry. That was poorly worded on my part. Some classes, like SnapshotTest.java and all the storage providers, did not get updated references to your refactoring. They still reference StrategyPriority.pickStrategy(), etc. Additionally, I changed the pickStrategy() logic from using a comparator to looping over the list once keeping a reference to the best result. This logic was lost in the merge.
>>
>> --
>> Chris Suich
>> chris.suich@netapp.com
>> NetApp Software Engineer
>> Data Center Platforms – Cloud Solutions
>> Citrix, Cisco & Red Hat
>>
>>> On Oct 23, 2013, at 4:13 PM, Darren Shepherd <da...@gmail.com> wrote:
>>>
>>> The transaction API was changed in the merge. I could have maybe
>>> missed updating a class. Let me check. When you said "It looks like
>>> the changes from us didn’t make it through your merge at all," can you
>>> point to something specific that got lost?
>>>
>>> Darren
>>>
>>> On Wed, Oct 23, 2013 at 1:05 PM, SuichII, Christopher
>>> <Ch...@netapp.com> wrote:
>>>> And it looks like some of your changes may have not merged correctly. I’m getting compile errors like:
>>>>
>>>> The method close() is undefined for the type Transaction
>>>>
>>>> This shouldn’t have come from our merge.
>>>>
>>>> --
>>>> Chris Suich
>>>> chris.suich@netapp.com
>>>> NetApp Software Engineer
>>>> Data Center Platforms – Cloud Solutions
>>>> Citrix, Cisco & Red Hat
>>>>
>>>>> On Oct 23, 2013, at 3:52 PM, Darren Shepherd <da...@gmail.com> wrote:
>>>>>
>>>>> Chris, Edison,
>>>>>
>>>>> You guys just committed 'Support Revert VM Disk from Snapshot.' At
>>>>> the same time I was merging both my txn-refactor and
>>>>> spring-modularization branches. They are really tricky merges and
>>>>> each time I have to rebase it takes awhile to figure out. Anyhow,
>>>>> your change + my changes breaks master. So I quickly rebased rb14823
>>>>> and committed to master. rb14823 is the patch that makes the Storage
>>>>> Strategies work with my spring work plus clean up some things.
>>>>> Additionally I found out you can't inject List<SnapshotStrategy> to
>>>>> the Snapshot object, so we really have to go with my change to
>>>>> centralize the ownership of the strategies to a single class.
>>>>>
>>>>> Can you please pull master and revalidate that I didn't break
>>>>> anything, if its not too much of a pain.
>>>>>
>>>>> Thanks,
>>>>> Darren
>>
Re: StrategyPriority changes w/ Spring Changes
Posted by Darren Shepherd <da...@gmail.com>.
Okay let me look at that. Are you 100% sure your looking at a clean version of master?
Darren
> On Oct 23, 2013, at 1:17 PM, "SuichII, Christopher" <Ch...@netapp.com> wrote:
>
> Er, sorry. That was poorly worded on my part. Some classes, like SnapshotTest.java and all the storage providers, did not get updated references to your refactoring. They still reference StrategyPriority.pickStrategy(), etc. Additionally, I changed the pickStrategy() logic from using a comparator to looping over the list once keeping a reference to the best result. This logic was lost in the merge.
>
> --
> Chris Suich
> chris.suich@netapp.com
> NetApp Software Engineer
> Data Center Platforms – Cloud Solutions
> Citrix, Cisco & Red Hat
>
>> On Oct 23, 2013, at 4:13 PM, Darren Shepherd <da...@gmail.com> wrote:
>>
>> The transaction API was changed in the merge. I could have maybe
>> missed updating a class. Let me check. When you said "It looks like
>> the changes from us didn’t make it through your merge at all," can you
>> point to something specific that got lost?
>>
>> Darren
>>
>> On Wed, Oct 23, 2013 at 1:05 PM, SuichII, Christopher
>> <Ch...@netapp.com> wrote:
>>> And it looks like some of your changes may have not merged correctly. I’m getting compile errors like:
>>>
>>> The method close() is undefined for the type Transaction
>>>
>>> This shouldn’t have come from our merge.
>>>
>>> --
>>> Chris Suich
>>> chris.suich@netapp.com
>>> NetApp Software Engineer
>>> Data Center Platforms – Cloud Solutions
>>> Citrix, Cisco & Red Hat
>>>
>>>> On Oct 23, 2013, at 3:52 PM, Darren Shepherd <da...@gmail.com> wrote:
>>>>
>>>> Chris, Edison,
>>>>
>>>> You guys just committed 'Support Revert VM Disk from Snapshot.' At
>>>> the same time I was merging both my txn-refactor and
>>>> spring-modularization branches. They are really tricky merges and
>>>> each time I have to rebase it takes awhile to figure out. Anyhow,
>>>> your change + my changes breaks master. So I quickly rebased rb14823
>>>> and committed to master. rb14823 is the patch that makes the Storage
>>>> Strategies work with my spring work plus clean up some things.
>>>> Additionally I found out you can't inject List<SnapshotStrategy> to
>>>> the Snapshot object, so we really have to go with my change to
>>>> centralize the ownership of the strategies to a single class.
>>>>
>>>> Can you please pull master and revalidate that I didn't break
>>>> anything, if its not too much of a pain.
>>>>
>>>> Thanks,
>>>> Darren
>
Re: StrategyPriority changes w/ Spring Changes
Posted by "SuichII, Christopher" <Ch...@netapp.com>.
Er, sorry. That was poorly worded on my part. Some classes, like SnapshotTest.java and all the storage providers, did not get updated references to your refactoring. They still reference StrategyPriority.pickStrategy(), etc. Additionally, I changed the pickStrategy() logic from using a comparator to looping over the list once keeping a reference to the best result. This logic was lost in the merge.
--
Chris Suich
chris.suich@netapp.com
NetApp Software Engineer
Data Center Platforms – Cloud Solutions
Citrix, Cisco & Red Hat
On Oct 23, 2013, at 4:13 PM, Darren Shepherd <da...@gmail.com> wrote:
> The transaction API was changed in the merge. I could have maybe
> missed updating a class. Let me check. When you said "It looks like
> the changes from us didn’t make it through your merge at all," can you
> point to something specific that got lost?
>
> Darren
>
> On Wed, Oct 23, 2013 at 1:05 PM, SuichII, Christopher
> <Ch...@netapp.com> wrote:
>> And it looks like some of your changes may have not merged correctly. I’m getting compile errors like:
>>
>> The method close() is undefined for the type Transaction
>>
>> This shouldn’t have come from our merge.
>>
>> --
>> Chris Suich
>> chris.suich@netapp.com
>> NetApp Software Engineer
>> Data Center Platforms – Cloud Solutions
>> Citrix, Cisco & Red Hat
>>
>> On Oct 23, 2013, at 3:52 PM, Darren Shepherd <da...@gmail.com> wrote:
>>
>>> Chris, Edison,
>>>
>>> You guys just committed 'Support Revert VM Disk from Snapshot.' At
>>> the same time I was merging both my txn-refactor and
>>> spring-modularization branches. They are really tricky merges and
>>> each time I have to rebase it takes awhile to figure out. Anyhow,
>>> your change + my changes breaks master. So I quickly rebased rb14823
>>> and committed to master. rb14823 is the patch that makes the Storage
>>> Strategies work with my spring work plus clean up some things.
>>> Additionally I found out you can't inject List<SnapshotStrategy> to
>>> the Snapshot object, so we really have to go with my change to
>>> centralize the ownership of the strategies to a single class.
>>>
>>> Can you please pull master and revalidate that I didn't break
>>> anything, if its not too much of a pain.
>>>
>>> Thanks,
>>> Darren
>>
Re: StrategyPriority changes w/ Spring Changes
Posted by Darren Shepherd <da...@gmail.com>.
The transaction API was changed in the merge. I could have maybe
missed updating a class. Let me check. When you said "It looks like
the changes from us didn’t make it through your merge at all," can you
point to something specific that got lost?
Darren
On Wed, Oct 23, 2013 at 1:05 PM, SuichII, Christopher
<Ch...@netapp.com> wrote:
> And it looks like some of your changes may have not merged correctly. I’m getting compile errors like:
>
> The method close() is undefined for the type Transaction
>
> This shouldn’t have come from our merge.
>
> --
> Chris Suich
> chris.suich@netapp.com
> NetApp Software Engineer
> Data Center Platforms – Cloud Solutions
> Citrix, Cisco & Red Hat
>
> On Oct 23, 2013, at 3:52 PM, Darren Shepherd <da...@gmail.com> wrote:
>
>> Chris, Edison,
>>
>> You guys just committed 'Support Revert VM Disk from Snapshot.' At
>> the same time I was merging both my txn-refactor and
>> spring-modularization branches. They are really tricky merges and
>> each time I have to rebase it takes awhile to figure out. Anyhow,
>> your change + my changes breaks master. So I quickly rebased rb14823
>> and committed to master. rb14823 is the patch that makes the Storage
>> Strategies work with my spring work plus clean up some things.
>> Additionally I found out you can't inject List<SnapshotStrategy> to
>> the Snapshot object, so we really have to go with my change to
>> centralize the ownership of the strategies to a single class.
>>
>> Can you please pull master and revalidate that I didn't break
>> anything, if its not too much of a pain.
>>
>> Thanks,
>> Darren
>
Re: StrategyPriority changes w/ Spring Changes
Posted by "SuichII, Christopher" <Ch...@netapp.com>.
And it looks like some of your changes may have not merged correctly. I’m getting compile errors like:
The method close() is undefined for the type Transaction
This shouldn’t have come from our merge.
--
Chris Suich
chris.suich@netapp.com
NetApp Software Engineer
Data Center Platforms – Cloud Solutions
Citrix, Cisco & Red Hat
On Oct 23, 2013, at 3:52 PM, Darren Shepherd <da...@gmail.com> wrote:
> Chris, Edison,
>
> You guys just committed 'Support Revert VM Disk from Snapshot.' At
> the same time I was merging both my txn-refactor and
> spring-modularization branches. They are really tricky merges and
> each time I have to rebase it takes awhile to figure out. Anyhow,
> your change + my changes breaks master. So I quickly rebased rb14823
> and committed to master. rb14823 is the patch that makes the Storage
> Strategies work with my spring work plus clean up some things.
> Additionally I found out you can't inject List<SnapshotStrategy> to
> the Snapshot object, so we really have to go with my change to
> centralize the ownership of the strategies to a single class.
>
> Can you please pull master and revalidate that I didn't break
> anything, if its not too much of a pain.
>
> Thanks,
> Darren