Thanks Hugo, I'm all for it if John can help us with his -1 vote :)
> -----Original Message----- > From: Trippie [mailto:trip...@gmail.com] On Behalf Of Hugo Trippaers > Sent: Thursday, June 27, 2013 5:51 PM > To: dev@cloudstack.apache.org > Subject: Re: [MERGE] Merge VMSync improvement branch into master > > > I think Ilya offers is great, my current stance is also to see how we can > bring > this forward. > > I've had the opportunity to meet with several people at the Citrix office in > Santa Clara, i'm actually working from their office at this moment. I think > it's > also the responsibility of someone who put in a -1 to work with the original > committer to get the situation resolved. So i'll invest the time to help with > the review as well. > > It would be great if Alex or Kelven could take the time to explain how this > feature has been tested. That would give the community some insight as > well. > > My main technical problem with this merge is that stuff is moving all over the > place without having even the slightest idea why. Now having discussed this > with Alex in person i get the general idea of this merge, so can actually try > to > review it. > > I think that John have nicely explained what we could do to prevent > situations like this in advance. I fully understand that big features or > rewrites > don't happen overnight and might show up near the end of the release cycle. > With the time based release cycle it's always a risk that some feature might > not make it in on time. Getting more people involved and chunking the > commits into master will greatly speed up the reviewing process. > > I'll get back to this after spending some time on reviewing the actual patch. > In > fact i would like to ask more people to have a look at this patch and reply to > this thread with comments or remarks. > > Cheers, > > Hugo > > > On Jun 27, 2013, at 12:55 PM, John Burwell <jburw...@basho.com> wrote: > > > 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. > > > > 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? > >>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>> > >>>> > >> > >> > > >
