> -----Original Message----- > From: John Burwell [mailto:jburw...@basho.com] > Sent: Thursday, June 27, 2013 12:55 PM > To: dev@cloudstack.apache.org > Subject: Re: [MERGE] Merge VMSync improvement branch into master > > Ilya, > > I understand your concern. However, this patch is large and > complicated. Without adequate review, we run that the risk getting it > wrong which would be worse than not having it. If you feel it is a must > have, you can propose extending the freeze to allow time to perform the > review. [Animesh>] IMO I would not like to extend the freeze date second time around and sticking to time based release. Ilya if this does not make to 4.2 then next opportunity would be 4.3 which will be 4 months apart not 8 as you mention. May be we should consider if there are important logical chunks that can be included for 4.2 rather than all of it.
> > Thanks, > -John > > On Jun 27, 2013, at 3:49 PM, "Musayev, Ilya" <imusa...@webmd.net> wrote: > > > 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? > >>>>>>>> > >>>>>>> > >>>>>> > >>>> > >>> > > > >
