This looks like a great strategy guys. Rohit, personally helping this week will be exceptionally difficult (i.e. probably not going to happen). That being said, heres an idea: Perhaps if you have an punch list to track progress, others with time can know where to spend it? Send updates to the list periodically to remind folks about what you are doing?
- chip Sent from my iPhone. On Dec 23, 2012, at 4:57 AM, Rohit Yadav <[email protected]> wrote: > The strategy now is that Prasanna will help resurrect host/hypervisor > simulator (the plugin is almost there, right now this would work, mvn clean > install -Psimulator, more on this later) and lead to write (both of us will > write tests, but I'm still a n00b and others pl. chip in and help with domain > specific stuff for. ex. apis like autoscaling etc.) OTW tests using marvin > and the simulator; so once we can validate the contract of basic set of APIs > on master, we can merge api_refactoring and test the contract after the merge > and fix any issues. > > Nitin, what do you mean by breaking master? Build won't be broken, nor any > features; the contract of API behaviour based on received params can be an > issue only when instead of a uuid string, a long int is passed which is the > only issue I can see. So, only thing that can break is the contract of APIs > (Cmd and Response) and any clients using them (UI, cloudmonkey, automation > scripts). Since 3.x you're only supposed to use uuid strings and not the > internal ids, if someone's using them, it's about time, they need to fix it > and with 4.1 we want to enforce that. > > Nevertheless, we'll have tests, you can see some simulator etc. on master: > https://git-wip-us.apache.org/repos/asf?p=incubator-cloudstack.git;a=search;s=Prasanna+Santhanam;st=author > > I really want to encourage everyone to participate in anything/any-work in > CloudStack, so the remark that a someone will be working on 4.1 (or xyz > feature) and community should not expect their members to help fix some > problem is not even an argument to begin with. > > You see a problem, you rant, bring on ML (maybe flame couple of culprits) and > send a patch, or as a community member/contributor/any-damn-dudeness-title > fix it. > > Regards. > > On 22-Dec-2012, at 10:49 PM, Nitin Mehta <[email protected]> wrote: > >> Hi Rohit, >> >> Please find my answers inline. >> >> On 23-Dec-2012, at 2:05 AM, Rohit Yadav wrote: >> >>> Hi David, >>> >>> Thanks for the email, yes I really need that to know what mistakes we are >>> doing, some of them are ack'd (we need tests and habit of contributing code >>> with tests) but for the most part you got the wrong message. >>> To say it again, refactoring work does not introduce any new functionality, >>> we're just trying to cleanup code, fix behaviour and fix apidocs etc. That >>> too restricted to cloud-api artifact. >>> >>> The only reason I want to do it now is because of the dependencies that >>> would have created a lot of merge conflicts for everyone else who would be >>> working now. And yes, this is WIP because I want to move everything in >>> cloud-api to org.apache.cloudstack, which if I do it now would cause huge >>> conflicts. For ex. there is no rule where all interfaces should be put, >>> most of them are in cloud-api, but some are spread across cloud-core, >>> cloud-server etc. It makes it difficult for a developer to understand the >>> arrange of code in a filesystem and see the big picture. >>> >>> Lastly, I don't know how to write (good) tests and the best methods of >>> testing but I think what we need is OTW tests, I've no idea how unit tests >>> for cmd/response classes would help (sorry unit test n00b here, not much >>> idea). >>> Testing would be done manually for now and/or using marvin's testclient >>> which Prasanna (marvin maintainer) can comment on. >>> Since, all the work is in api_refactoring, it's not in the limelight so >>> what I want is to bring these changes to master so more folks can >>> participate. And, yes I'm trying to break stuff so people who really care >>> would help fix it (if anyone remembered, I liked Hugo's approach during >>> Maven migration time, if anyone remembers the epic email something like >>> it's better to break stuff and get folks involved then have it buried...). >>> At the same time, I'm fixing stuff and not just breaking them and leaving >>> it for anyone else to fix it. >>> >>> If this is not fine, how about plan B: I'll merge master on api_refactoring >>> to get the latest stuff, fix conflicts, move stuff but then anyone who >>> wishes to contribute tests/code in api-layer (how would be ensure or >>> enforce that? one of the reasons I want this to be merged on master) should >>> send reviews/patches for api_refactoring branch. When we'll have tests etc. >>> we can try again for a merge request? >> >> I suggest you go with Plan B here. A lot of folks will be working on 4.1 >> features. You shouldn't expect them to leave their feature work and help >> fixing this. >> I DONT like the idea of breaking master. This should totally be the >> responsibility of the refactoring dude :). Yes, there will be a day or two >> of unstability when you finally merge the code because you can't keep in >> sync with master, but that should be the strategy. >> >>> Regards. >>> PS. Refactoring work, manual hunting and pecking about 300 apis, cleaning >>> mistakes of the past working day n night, even on weekends was not fun, >>> just sayin' >>> PPS. The motivation was that it would make it easier for a contributor to >>> contribute code, no "bad form" intended >>> PPPS. Where are the conf. videos? >>> >>> ________________________________________ >>> From: David Nalley [[email protected]] >>> Sent: Saturday, December 22, 2012 9:14 PM >>> To: [email protected] >>> Subject: Re: Merge request: Merging api_refactoring on master >>> >>> On Sat, Dec 22, 2012 at 4:38 AM, Rohit Yadav <[email protected]> wrote: >>>> Hi everyone, >>>> >>>> I'm planning to merge api_refactoring branch on to master after 72 hour >>>> period which would be Monday EOD. Pl. go through the email, and previous >>>> threads on api refactoring rework and feel free to share your ideas, >>>> comments and vote to agree, disagree. If no one objects I would like to >>>> ask the git Santa to merge it on Christmas 25 Dec :D (after 72 hour window) >>>> >>>> The reason why I want to merge around the next week is because I think we >>>> would have lower frequencies of emails, review requests and people >>>> contributing, hence I can move around a lot of code (mostly package >>>> renames to org.apache.cloudstack in cloud-api) and right now the merge >>>> conflicts are really minimum, about 100-200 lines. (A top level issues to >>>> track sub-issues: https://issues.apache.org/jira/browse/CLOUDSTACK-638) >>> >>> >>> >>> So yes - 72 hours is the minimum. However springing this on folks when >>> a good portion of the world has begun celebrating a holiday and is >>> hopefully ignoring mailing lists is incredibly bad form. This should >>> at least wait until the first week of January IMO due to the magnitude >>> of the change. This is incredibly invasive, and based on your comments >>> below, still a work in progress. >>> >>> >>> >>>> What will be affected: >>>> 0. Any class in cloud-api and on api-layer only >>>> 1. Any class that imports from/to cloud-api's response and cmd classes >>>> >>>> Some of the major changes that will be merged on master; >>>> 0. Over the wire (OTW) HTTP request to API server would send only UUID >>>> strings. All requests done via UUIDs (and not CloudStack's internal db's >>>> IDs). >>>> 1. https://issues.apache.org/jira/browse/CLOUDSTACK-518 Fix @Parameter >>>> annotation to have annotation field to a Response class which would give >>>> us entities (interface to VO objects). This would get rid of all >>>> IdentityMapper using which was used earlier to get VO entities from an >>>> annotated table name. This helps us to translate OTW UUIDs to CloudStack's >>>> DB's internal IDs. >>>> 2. Separation of ACL Role access checker as an adapter, so organizations >>>> can implement their own role based access checking. The mechanism would >>>> exist in CloudStack's API server but policy checking is moved out of >>>> CloudStack. (https://issues.apache.org/jira/browse/CLOUDSTACK-639) This >>>> works, but was tough to get it right the first time, there is better way >>>> which I'll share before the merge. >>>> 3. Group APIs to >>>> org.apache.cloudstack.api.{command,response}.{entity1,entity2 etc.} >>>> packages. This is mainly done for developers, so when they work on API >>>> layer they would know which api has what level of security and as they are >>>> grouped based on entity type, it will be easier to search. This was mostly >>>> file movement to org.apache.cloudstack.api package and helped us track >>>> couple of classes which are no longer needed. Another aim was to move from >>>> com.cloud to org.apache.cloudstack (only cloud-api for now). >>>> 4. Annotation work as described in 1., also for @ACL etc. >>>> 5. DB, ACL validation wip code >>>> 6. A lot of list api optimizations and response view work from our newest >>>> commiter, Min Chen. The aim is to simply response, right now for. example >>>> when we listVMs we don't want unnecessary (serialized) response objects >>>> which could be queried using uuids separately. >>>> >>>> Pl. ask away any doubts, questions and concerns you may have. It was >>>> challenging for me as well to understand the functional spec, to know the >>>> why/what/how, and if you read the old threads you can tell I did not get >>>> it the first time. >>>> >>>> A lot of annotation work is aimed to be completed over this weekend, so >>>> when the branch is finally merged it won't break any functionality. At >>>> present the branch is quite stable >>> >>> >>> >>>> Testing and how or why do it? >>>> 0. Prasanna, Meghna? can help us write few basic sets of unit tests and >>>> marvin integration tests for OTW requests. We already have few of their >>>> patches on rb. >>> >>> You are proposing to merge changes to master for the primary way of >>> interacting with CloudStack and have not rests written? >>> >>>> 1. We can also have drivers to automate tests (Prasanna can talk more on >>>> this and on his devcloud based continuous intergration server) >>>> 2. If I do it now, there would be a lot more eyes to point out bugs and I >>>> want more people to participate in the refactoring work. >>> >>> Is this really: 'I can break it now and make more people help me clean it >>> up?' >>> >>>> 3. Right now, it builds and runs fine with minimal breaks and no >>>> functionality breakage as most of the changes are only restricted to >>>> api-server (:cloud-api artifact). I'm able to deploy a zone etc. To make >>>> the UUID thing work, I've put in hardcoded (for. ex. projectId=-1 which >>>> should be a string uuid not a long int value -1) stuff that saves the UI >>>> from being broken which I'll remove after merging on master so UI >>>> engineers can help fix UI issues. >>> >>> API compatibility between versions is a gigantic concern for me (and >>> hopefully anyone else using or working on ACS). I have asked >>> previously about what the plans were around testing this [1] and >>> received no answer, and judging from the above, it appears there >>> aren't even tests written yet. IMO that is a blocker. I am also >>> disappointed that despite touching tons of code in this refactoring >>> effort, virtually zero unit tests were added. However the concern >>> around API compatibility and lack of testing to show that it isn't >>> broken are going to lead me to cast a -1 (binding veto). I am not >>> inherently opposed to this work, but I am opposed to merging it in >>> untested and without tests itself. We've previously discussed that >>> test are a part of a feature being considered complete and ready for >>> merge. >>> >>> >>> [1] >>> http://mail-archives.apache.org/mod_mbox/incubator-cloudstack-dev/201212.mbox/%3CCAKprHVbdcm1Ot2v%3DH8zCHmHhgX5iS3OyoVgH8Vh7XOmXhmBX5w%40mail.gmail.com%3E > >
