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 bre

Re: StrategyPriority changes w/ Spring Changes

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 S

Re: StrategyPriority changes w/ Spring Changes

I can fix it, let me see what I broke. Darren > On Oct 23, 2013, at 1:03 PM, "SuichII, Christopher" > 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 fr

Re: StrategyPriority changes w/ Spring Changes

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

Re: StrategyPriority changes w/ Spring Changes

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, SuichI

Re: StrategyPriority changes w/ Spring Changes

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 comparato

Re: StrategyPriority changes w/ Spring Changes

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" > wrote: > > Er, sorry. That was poorly worded on my part. Some classes, like > SnapshotTest.java and all the storage providers, did not get up

Re: StrategyPriority changes w/ Spring Changes

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 wrote: > Okay let me look at that. Are you 100% sure your looking at a clean version

Re: StrategyPriority changes w/ Spring Changes

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 wrote: > Yep. I’m running on a clean master. > > -- > Chris Suich > chris.su...@netapp.

Re: StrategyPriority changes w/ Spring Changes

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 wrote:

Re: StrategyPriority changes w/ Spring Changes

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

Re: StrategyPriority changes w/ Spring Changes

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 wrote: > Darren, > > Would you be able to look into copy the logic back in

Re: StrategyPriority changes w/ Spring Changes

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

Re: StrategyPriority changes w/ Spring Changes

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

Re: StrategyPriority changes w/ Spring Changes

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 Su

Re: StrategyPriority changes w/ Spring Changes

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 wrote: > If there are arguments against it, then lets keep the discussion going. I’m > fine with sorting as

Re: StrategyPriority changes w/ Spring Changes

Updated f3e968b9830a667cb3b55477f1fe8259e56ceffb Darren On Wed, Oct 23, 2013 at 4:23 PM, Darren Shepherd 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, Suich