Chip - Thanks for looking into this. Please find my answers inline. On 18/03/13 10:34 PM, "Chip Childers" <chip.child...@sungard.com> wrote:
>On Mon, Mar 18, 2013 at 04:10:25PM +0000, Nitin Mehta wrote: >> I would like to merge scale up vm functionality branch to master >> >> Spec : >> >>https://cwiki.apache.org/confluence/display/CLOUDSTACK/Dynamic+scaling+of >>+CPU+and+RAM >> >> JIRA ticket : >> https://issues.apache.org/jira/browse/CLOUDSTACK-658 >> >> Branch: >> Scaleupvm >> >> Notes:- >> >> 1. I have removed the vm state change in the code and so there is no >>vm state machine change. I am planning to achieve the synchronization >>using async job framework. I will make this change once I get a reply >>from Kelvn (minor change) >> 2. With the changes in #1 this feature is independent and should not >>be intrusive to any functionality. >> >> Thanks, >> -Nitin > >Nitin, > >A couple of notes: > >1 - The license header is misplaced in ScaleVMCmd.java Corrected thanks. >2 - Thanks for adding unit tests for ScaleVMCmd and UserVMManagerImpl >3 - I would have expected to see unit tests for the new methods in >CitrixResourceBase and VirtualMachineManagerImpl I have added unit tests for VirtualMachineManagerImpl but in CitrixResourceBase its just a resource call (here the Xenserver) and so I don't see benefit in writing a unit test. I am not adding unit test here. >4 - Should we have some marvin tests for this? We can but this is a fairly isolated functionality and I don't see much use in writing this. Nevertheless I would love to get my hands dirty for writing one. ATM though I would rather get the code out in the community and then do this work rather than holding the feature push. I am also a Python newbie so it will take some time. >
