Mailing-List: contact cloudstack-dev-help@incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: cloudstack-dev@incubator.apache.org
Received-SPF: neutral (athena.apache.org: local policy)
MIME-Version: 1.0
In-Reply-To: 
 <0BCCCE152323764BB7FD6AE6D7A1D9060102894ECFFE@BANPMAILBOX01.citrite.net>
References: 
 <0BCCCE152323764BB7FD6AE6D7A1D9060102894ECFFE@BANPMAILBOX01.citrite.net>
From: David Nalley <david@gnsa.us>
Date: Sat, 22 Dec 2012 10:44:11 -0500
Message-ID: 
 <CAKprHVZjLOH0iOuAo9_v3z_EDzWmCaY3rtU545BMLB0DELr3dQ@mail.gmail.com>
Subject: Re: Merge request: Merging api_refactoring on master
To: cloudstack-dev@incubator.apache.org
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable

On Sat, Dec 22, 2012 at 4:38 AM, Rohit Yadav <rohit.yadav@citrix.com> wrote=
:
> Hi everyone,
>
> I'm planning to merge api_refactoring branch on to master after 72 hour p=
eriod which would be Monday EOD. Pl. go through the email, and previous thr=
eads 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 contrib=
uting, 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 real=
ly minimum, about 100-200 lines. (A top level issues to track sub-issues: h=
ttps://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 st=
rings. All requests done via UUIDs (and not CloudStack's internal db's IDs)=
.
> 1. https://issues.apache.org/jira/browse/CLOUDSTACK-518 Fix @Parameter an=
notation to have annotation field to a Response class which would give us e=
ntities (interface to VO objects). This would get rid of all IdentityMapper=
 using which was used earlier to get VO entities from an annotated table na=
me. 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 exi=
st in CloudStack's API server but policy checking is moved out of CloudStac=
k. (https://issues.apache.org/jira/browse/CLOUDSTACK-639) This works, but w=
as tough to get it right the first time, there is better way which I'll sha=
re before the merge.
> 3. Group APIs to org.apache.cloudstack.api.{command,response}.{entity1,en=
tity2 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 t=
hey 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 tr=
ack couple of classes which are no longer needed. Another aim was to move f=
rom 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 whi=
ch could be queried using uuids separately.
>
> Pl. ask away any doubts, questions and concerns you may have. It was chal=
lenging for me as well to understand the functional spec, to know the why/w=
hat/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 w=
hen the branch is finally merged it won't break any functionality. At prese=
nt 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 m=
arvin integration tests for OTW requests. We already have few of their patc=
hes 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 u=
p?'

> 3. Right now, it builds and runs fine with minimal breaks and no function=
ality breakage as most of the changes are only restricted to api-server (:c=
loud-api artifact). I'm able to deploy a zone etc. To make the UUID thing w=
ork, I've put in hardcoded (for. ex. projectId=3D-1 which should be a strin=
g uuid not a long int value -1) stuff that saves the UI from being broken w=
hich I'll remove after merging on master so UI engineers can help fix UI is=
sues.
>

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/20121=
2.mbox/%3CCAKprHVbdcm1Ot2v%3DH8zCHmHhgX5iS3OyoVgH8Vh7XOmXhmBX5w%40mail.gmai=
l.com%3E