Updated f3e968b9830a667cb3b55477f1fe8259e56ceffb
Darren
On Wed, Oct 23, 2013 at 4:23 PM, Darren Shepherd
<darren.s.sheph...@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
> <chris.su...@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.su...@netapp.com
>> NetApp Software Engineer
>> Data Center Platforms – Cloud Solutions
>> Citrix, Cisco & Red Hat
>>
>> On Oct 23, 2013, at 6:39 PM, Darren Shepherd <darren.s.sheph...@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
>>> <chris.su...@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.su...@netapp.com
>>>> NetApp Software Engineer
>>>> Data Center Platforms – Cloud Solutions
>>>> Citrix, Cisco & Red Hat
>>>>
>>>> On Oct 23, 2013, at 6:25 PM, Darren Shepherd <darren.s.sheph...@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
>>>> <chris.su...@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.su...@netapp.com
>>>> NetApp Software Engineer
>>>> Data Center Platforms – Cloud Solutions
>>>> Citrix, Cisco & Red Hat
>>>>
>>>> On Oct 23, 2013, at 5:56 PM, SuichII, Christopher <chris.su...@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.su...@netapp.com
>>>> NetApp Software Engineer
>>>> Data Center Platforms – Cloud Solutions
>>>> Citrix, Cisco & Red Hat
>>>>
>>>> On Oct 23, 2013, at 5:48 PM, Darren Shepherd <darren.s.sheph...@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
>>>> <chris.su...@netapp.com> wrote:
>>>>
>>>> Yep. I’m running on a clean master.
>>>>
>>>> --
>>>> Chris Suich
>>>> chris.su...@netapp.com
>>>> NetApp Software Engineer
>>>> Data Center Platforms – Cloud Solutions
>>>> Citrix, Cisco & Red Hat
>>>>
>>>> On Oct 23, 2013, at 4:30 PM, Darren Shepherd <darren.s.sheph...@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"
>>>> <chris.su...@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.su...@netapp.com
>>>> NetApp Software Engineer
>>>> Data Center Platforms – Cloud Solutions
>>>> Citrix, Cisco & Red Hat
>>>>
>>>> On Oct 23, 2013, at 4:13 PM, Darren Shepherd <darren.s.sheph...@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
>>>> <chris.su...@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.su...@netapp.com
>>>> NetApp Software Engineer
>>>> Data Center Platforms – Cloud Solutions
>>>> Citrix, Cisco & Red Hat
>>>>
>>>> On Oct 23, 2013, at 3:52 PM, Darren Shepherd <darren.s.sheph...@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
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>
