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
