Alex, +1 for breaking up into smaller patches. I missed an opportunity to express my admiration for your withdrawal of the patch for 4.2. I imagine it was hard decision given all of the work you poured into it, and I think your action stands a great example of engineering integrity and quality focus for the community.
Thanks, -John On Jul 3, 2013, at 12:22 PM, Alex Huang <alex.hu...@citrix.com> wrote: > While I'm working on the BVT issues to be resolved. I will attempt to pull > some of the less architectural changes over to master in pieces. That way > when people do have to review the VMSync code, the code difference will be > much smaller. Hopefully, the merges will be smaller too. > > For these changes, I will not be asking for review requests. They're > refactoring done by Eclipse.. > > --Alex > >> -----Original Message----- >> From: Alex Huang [mailto:alex.hu...@citrix.com] >> Sent: Monday, July 1, 2013 10:20 AM >> To: dev@cloudstack.apache.org >> Subject: RE: [MERGE] Merge VMSync improvement branch into master >> >> Ilya, >> >> Thanks! The current problem on this branch is we're trying to distinguish >> what's actually broken on master and what's broken by introducing this >> change. At this point, the branch is basically frozen and only applying bug >> fixes once BVT found problems. >> >> --Alex >> >>> -----Original Message----- >>> From: Musayev, Ilya [mailto:imusa...@webmd.net] >>> Sent: Monday, July 1, 2013 10:16 AM >>> To: dev@cloudstack.apache.org >>> Subject: RE: [MERGE] Merge VMSync improvement branch into master >>> >>> Alex, >>> >>> I completely understand. Please keep us in the loop on the progress. >>> If its functional to some extent - i.e. at least build succeeds >>> without errors and major functionality is there, I can merge the code >>> into CloudSand CS distro - to test it out and share it with whoever wants to >> try it. >>> >>> Thanks >>> ilya >>> >>>> -----Original Message----- >>>> From: Alex Huang [mailto:alex.hu...@citrix.com] >>>> Sent: Friday, June 28, 2013 9:16 PM >>>> To: dev@cloudstack.apache.org >>>> Subject: RE: [MERGE] Merge VMSync improvement branch into master >>>> >>>> Given the current state of BVT, I don't think we can reliably merge >>>> this into master. It will have to wait for 4.3. I apologize to >>>> those who really want to see this feature in. I myself have slaved >>>> over this for some weeks, including missing the collab conference, >>>> but I just cannot conscientiously push it in under the current >> circumstances. >>>> >>>> --Alex >>>> >>>>> -----Original Message----- >>>>> From: Sudha Ponnaganti [mailto:sudha.ponnaga...@citrix.com] >>>>> Sent: Friday, June 28, 2013 7:11 AM >>>>> To: dev@cloudstack.apache.org >>>>> Subject: RE: [MERGE] Merge VMSync improvement branch into master >>>>> >>>>> Ideally I would like to see all validation to be done on feature >>>>> branch - BVTs and also manual validation of the P1 test cases from >>>>> VM Sync test plan just like object store validation - sadhu has >>>>> signed up for it along with Ilya. This would help us to identify >>>>> feature >>> specific issues. >>>>> >>>>> We are running BVTs right now which have high failure rate. >>>>> Focusing on this part right now to reach parity with Master branch. >>>>> >>>>> >>>>> -----Original Message----- >>>>> From: Trippie [mailto:trip...@gmail.com] On Behalf Of Hugo >>>>> Trippaers >>>>> Sent: Thursday, June 27, 2013 5:32 PM >>>>> To: dev@cloudstack.apache.org >>>>> Subject: Re: [MERGE] Merge VMSync improvement branch into master >>>>> >>>>> I agree with John that a change like this is very hard to test in >>>>> an automated fashion. Still i have been looking at the numbers for >>>>> the code coverage with cobertura. I was a bit disappointed to find >>>>> that we have not made any progress with this merge with regards to >>>>> unit tests and >>>> total code coverage. >>>>> We do not seem to be in a worse shape than before. >>>>> >>>>> @Sudha, it would be nice if you could add your view on this patch >>>>> from the QA perspective? How would this patch affect your planning >>>>> for >>>> example? >>>>> >>>>> Cheers, >>>>> >>>>> Hugo >>>>> >>>>> On Jun 27, 2013, at 5:12 PM, John Burwell <jburw...@basho.com> >> wrote: >>>>> >>>>>> @David The types of concurrency changes introduced in this patch >>>>>> are extremely difficult to completely test in an automated fashion. >>>>>> Therefore, code review for correctness is critical to ensure quality. >>>>>> To be clear, I am not questioning the value of automated testing. >>>>>> I am just noting that it's next to impossible to achieve full >>>>>> coverage, and code review is an critical supplement. >>>>>> >>>>>> @Ilya I plan to review this patch, but I will be able to start >>>>>> until next week. I am also still reviewing object_store (a >>>>>> separate procedural issue for another thread), and need to >>>>>> complete >>> solidfire. >>>>>> This backlog is precisely why need to be reviewing iteratively >>>>>> throughout the dev cycle. >>>>>> >>>>>> Thanks, >>>>>> -John >>>>>> >>>>>> On Jun 27, 2013, at 7:35 PM, David Nalley <da...@gnsa.us> wrote: >>>>>> >>>>>>> On Thu, Jun 27, 2013 at 5:51 PM, Hugo Trippaers >>>>>>> <h...@trippaers.nl> >>>>> wrote: >>>>>>>> >>>>>>>> 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 >>>>>>> >>>>>>> So the problem in my mind, is that we don't have a way of >>>>>>> verifying that master isn't broken, and won't be broken by any >>>>>>> given merge. I look at even the minimal level of automated >>>>>>> testing that I see today, and ~20% of integration tests are >>>>>>> failing[1]. The regression set of tests (which isn't running as >>>>>>> often) is seeing 75% of tests failing[2]. Heaping on more >>>>>>> change when we are demonstrably already failing in many places >>>>>>> is not >>> behaving responsibly IMO. >>>>>>> The question I'd pose is this - running the various automated >>>>>>> tests is pretty cheap - whats the output of that compared to >>>>>>> the current test output on master? Better or worse? If it >>>>>>> hasn't been done, why >>>> not? >>>>>>> I desperately want these features, but not necessarily at the >>>>>>> cost of further destabilizing what we have now in master - we >>>>>>> can't continue accruing technical debt. >>>>>>> >>>>>>> --David >>>>>>> >>>>>>> [1] >>>>>>> http://jenkins.buildacloud.org/view/cloudstack-qa/job/test-smok >>>>>>> e- ma tr ix/lastCompletedBuild/testReport/ [2] >>>>>>> http://jenkins.buildacloud.org/view/cloudstack-qa/job/test-regr >>>>>>> es >>>>>>> si >>>>>>> on >>>>>>> -matrix/28/testReport/ >>>> >>> >
