Thanks Hugo. Let me bring some reasoning to these changes.
AsyncJobManager previously was embedded as part of the server package. It only served api jobs. In Kelven's design, all vm operations relies on asyncjob to order the operation so that it will no longer have any concurrentoperationexceptions for example. All HA jobs will also be funneled to the asyncjob. For that reason, asyncjob becomes a much more generic entity. For that we moved it into it's own jar package and remove all traces of api that's embedded inside asyncjobmanager. For each client of asyncjobmanager, they have to define their own dispatcher which does the serialization and deserialization of their jobs themselves. The dispatcher for api is called ApiAsyncJobDispatcher.java. The dispatcher for vm work is now called VmWorkJobDispatcher.java. Due to this change most of the cmd files in api package have to be changed because asyncjob topics and asyncjob command types were universally included in the *cmd files. The actual command execution were not changed. As for import, that's a function of my eclipse settings. It organizes our imports to this order: Java, javax, org, com, org.apache.cloudstack, com.cloud. This puts the cloudstack imports to the bottom. I don't think any review needs to be done on the imports. Please throw any questions you have on the list. Will be happy to explain. --Alex > -----Original Message----- > From: Trippie [mailto:trip...@gmail.com] On Behalf Of Hugo Trippaers > Sent: Thursday, June 27, 2013 4:27 PM > To: dev@cloudstack.apache.org > Subject: Re: [MERGE] Merge VMSync improvement branch into master > > Let's get it done :-) > > > I'm at about 15% of the entire commit, reading through it line by line... > > My notes so far (this is just a copy of my notepad fyi) > There are a lot of changes to BaseCmd and Async command. Do they > impact the api in any way. I can't tell yet but this might impact api > compatibility by fixing some of the current issues in the api. (Some external > programs like libcloud and jclouds have work-arounds for issues with the > current api) > > Lot of cleanup done for spelling errors and re-ordering of imports. > Typically something that we should push in as separate commits in master. > No risk and they make the patch unnecessarily bloated. > > UserContext has been replaced by CallContext and is used more > consistently. GoodThing((tm)) > > Netapp support enabled in the component context. Seems > unrelated to this particular patch? > > usageServerMonitor disabled in the component context. Also seems > unrelated to this particular patch? > > > I'll be updating this with my progress. > > Cheers, > > Hugo > > > On Jun 27, 2013, at 4:05 PM, "Musayev, Ilya" <imusa...@webmd.net> wrote: > > > 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? > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>> > >>>>>>> > >>>>>> > >>>> > >>>> > >>> > >> > > > >
