John and Hugo, I see and understand your concerns, however, this feature is really needed by many users - I've met with several large users at CCC13 who were looking forward to this work, blocking this merge - would delay cloudstack adoption and make ACS look inferior to other cloud platforms. Realistically, this is going to put us back at least 8+ months away until 4.3 comes out.
If needed, I can dedicate QA cycles and work with Kelven to rule out all the bugs and issues - through as many scenarios as possible on my end. Thanks ilya > -----Original Message----- > From: John Burwell [mailto:jburw...@basho.com] > Sent: Thursday, June 27, 2013 2:33 PM > To: dev@cloudstack.apache.org > Subject: Re: [MERGE] Merge VMSync improvement branch into master > > Hugo, > > I completely agree with this stance, and will add a -1 as well. It has been a > significant challenge this cycle to complete high quality reviews due to the > large patch size and short time turnaround time. Going forward, I am going > to be more likely to follow Hugo's example, and -1 patches for which there is > not adequate review time. I think the following techniques will help > streamline the review process: > > 1. Narrow Patch Scope: Ensure that the patch is confined to a single > enhancement or defect fix. If you doing refactoring as part of a feature, > submit the refactoring work a separate commit. As a further suggestion, if > you are performing multiple refactorings (e.g. package reorganization and > utility method extraction), split each refactoring into separate patches. > 2. Chunk Features: For each large feature being developed, logically chunk > the functionality and submit each chunk as a separate patch. I recommend > erring on the side of chunks being too small. Let the reviewer determine if > the patch is not large enough to be representative of the feature. The worst > that will happen is that a reviewer will provide feedback on the work > completed and simply ask for more work to be done before it can be merged > to master. For features while chunks can't be merged all the way to master, > intermediate patches can be submitted to Review Board for review > throughout the cycle -- allowing large merges to actually be the accumulation > of several smaller reviews. > 3. Identify Reviewers Early: For big features, solicit the reviewers whiling > to > review a feature from FS through merge from the mailing list. Reviews will > go much more quickly if reviewers have been involved from the beginning > because they will not help identify issues before coding starts, but also will > not have to develop context late. To be clear, identifying reviewers early > does not preclude a new reviewer from becoming involved later, but the > reviews will greatly reduce the probability of a late review finding > significant > issues. Additionally, the early reviewers can help the late reviewers come up > to speed -- reducing the burden on the contributor. > > Generally, my recommendation is to submit patches early and often. > Ultimately, contributors determine how they wish to develop and deliver > their contributions. These are only my recommendations as a reviewer to > increase the probability that a feature will make a particular release train. > Finally, it appears that reviewers are growing less and less tolerant of large > patches that appear just before freeze dates, and those these patches face a > much higher risk of being immediately receiving a -1 for the reasons stated > above. > > Thanks, > -John > > On Jun 26, 2013, at 8:45 PM, Hugo Trippaers <h...@trippaers.nl> wrote: > > > Hey Alex, Kelven, > > > > I've been looking though the code and thanks for the very insightfull > conversation and follow-up email. With merges of this magnitude it's > essential to help people understand what is going on. > > > > Purely technical this is a merge i'm really pleased with. It will for sure > improve our handling of virtual machines. > > > > However the timing of the merge request is not ideal. We are very close to > the end of the already extended feature freeze. We have a consensus within > the dev community to do large architectural changes in the beginning of the > cycle and not at the very end. This not only means a lot of extra testing > effort > and other developers will have to get used to the changes introduced right at > the moment when everybody should be focussed on bug fixing, > documentation and stability. Without wanting to rip open old wounds, i can > imagine we all want to avoid a javelin incident. > > > > I respect the work going into this and the effort it will take to keep this > > up > to date with the current speed of master, but still i'm going to veto this > with a > -1 for inclusion before we cut of the 4.2 branch. > > > > Cheers, > > > > Hugo > > > > On Jun 26, 2013, at 5:32 PM, Alex Huang <alex.hu...@citrix.com> wrote: > > > >> Hi All, > >> > >> Kelven had an emergency so I'm submitting the changes on vmsync for > >> him. The patch are on https://reviews.apache.org/r/12126. > >> > >> Hugo took a look and already had some questions on why so many files > were changed and added/deleted, So I like to explain a bit in this email. > >> > >> As part of the vmsync change, we try to move the files that were relevant > to vm orchestration into the right places so that others can properly view the > different jars and understand the relationship between the jars. This > process is ongoing and will continue in other changes. > >> > >> - Work related to jobs management have been put under cloud- > framework-jobs as handling jobs is not really part of our server but is a > library. By doing it this way, changes in it can be properly reviewed and > there's a good separation from things that utilizes jobs and jobs itself. > >> - VirtualMachine orchestration has been moved from cloud-server to > cloud-engine-orchestration. Cloud-engine-orchestration then only depends > on cloud-engine-schema and cloud-api. This creates a clear compilation > boundary between the self-service server implementation and orchestration > implementation. > >> > >> As part of these changes, then we surfaced many problems because we > setup the proper compilation boundaries and fixed all of these problems. > For example, HostAllocator interface is something people can write plugins > for but it was buried inside cloud-server package. We've moved a majority of > these changes to cloud-api. There's quite a bit of file movements based on > this change. For vmsync changes itself, the changes are centered around > VirtualMachineManagerImpl.java so you can review that file instead. > >> > >> Thanks. > >> > >> --Alex > >> > >> > >>> -----Original Message----- > >>> From: Kelven Yang [mailto:kelven.y...@citrix.com] > >>> Sent: Tuesday, June 18, 2013 3:38 PM > >>> To: dev@cloudstack.apache.org > >>> Subject: Re: [MERGE] Merge VMSync improvement branch into master > >>> > >>> > >>> > >>> On 6/17/13 7:43 PM, "Mice Xia" <mice_...@tcloudcomputing.com> > wrote: > >>> > >>>> Kelven, > >>>> > >>>> After the refactoring, will CS still restart HA enabled VM when it > >>>> is power off externally (e.g. using xencenter) or internally (user > >>>> initiated shutdown or crash)? > >>> > >>> > >>> If HA functionality is provided by external manager (i.e., vCenter), > >>> CS won't try to restart it, but if HA is provided by CloudStack, we > >>> will restore the legacy logic. The hook up with old HA manager > >>> (between > >>> HighAvailabilityManager/VirtualMachineManager) is in the wrap-up > >>> process > >>> > >>> > >>>> > >>>> Is seems the method HighAvailabilityManagerImpl .scheduleRestart() > >>>> is not called when VM's actual state is stopped while expected > >>>> state is > >>> running. > >>>> > >>>> > >>>> Regards > >>>> Mice > >>>> > >>>> -----Original Message----- > >>>> From: Kelven Yang [mailto:kelven.y...@citrix.com] > >>>> Sent: Tuesday, June 18, 2013 5:21 AM > >>>> To: dev@cloudstack.apache.org > >>>> Subject: Re: [MERGE] Merge VMSync improvement branch into master > >>>> > >>>> Haven't created a patch yet, will do it soon after some last wrap-ups. > >>>> > >>>> Kelven > >>>> > >>>> On 6/17/13 12:03 PM, "John Burwell" <jburw...@basho.com> wrote: > >>>> > >>>>> Kelven, > >>>>> > >>>>> Did this patch get pushed to Review Board? If so, what is the URL? > >>>>> > >>>>> Thanks. > >>>>> -John > >>>>> > >>>>> On Jun 17, 2013, at 1:40 PM, Kelven Yang <kelven.y...@citrix.com> > wrote: > >>>>> > >>>>>> Low level classes were tested in unit tests(MessageBus, Job > >>>>>> framework, Job dispatchers etc), interface layer changes are > >>>>>> guarded through matching the old semantics, but changes are > >>>>>> tested manually, we are planning to get this part of testing > >>>>>> through BVT system after we have re-based the latest master. > >>>>>> > >>>>>> Kelven > >>>>>> > >>>>>> On 6/17/13 10:01 AM, "Chip Childers" <chip.child...@sungard.com> > >>> wrote: > >>>>>> > >>>>>>> On Mon, Jun 17, 2013 at 04:59:00PM +0000, Kelven Yang wrote: > >>>>>>>> I'd like to kick off the official merge process. We will start > >>>>>>>> the merge process after the branch has passed necessary tests > >>>>>>>> > >>>>>>>> Kelven > >>>>>>> > >>>>>>> Can you share what testing is being run against the branch? > >>>>>> > >>>>> > >>>> > >> > >
