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(™)
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?
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>>>
>>>
>>
>
>
