Mailing-List: contact cloudstack-dev-help@incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: cloudstack-dev@incubator.apache.org
Received-SPF: pass (athena.apache.org: domain of jburwell@basho.com designates
 209.85.214.171 as permitted sender)
References: 
 <0BCCCE152323764BB7FD6AE6D7A1D9060102894ECFFE@BANPMAILBOX01.citrite.net>
 <CAKprHVZjLOH0iOuAo9_v3z_EDzWmCaY3rtU545BMLB0DELr3dQ@mail.gmail.com>
From: John Burwell <jburwell@basho.com>
Mime-Version: 1.0 (1.0)
In-Reply-To: 
 <CAKprHVZjLOH0iOuAo9_v3z_EDzWmCaY3rtU545BMLB0DELr3dQ@mail.gmail.com>
Date: Sat, 22 Dec 2012 14:08:33 -0500
Message-ID: <-4397755056359766803@unknownmsgid>
Subject: Re: Merge request: Merging api_refactoring on master
To: "cloudstack-dev@incubator.apache.org"
 <cloudstack-dev@incubator.apache.org>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable

David,

+1 to your concerns regarding timing and scope.  I also wonder what
impact this will have to Marvin, and that those impacts have been
thought tested.  We are entering a period where we are focused on
testing features leading up the 31Jan 2013 deadline.  I am concern
that Marvin will be unstable leading up the 4.1.0 deadline -- delaying
feature testing.

Thanks,
-John

On Dec 22, 2012, at 10:44 AM, David Nalley <david@gnsa.us> wrote:

> On Sat, Dec 22, 2012 at 4:38 AM, Rohit Yadav <rohit.yadav@citrix.com> wro=
te:
>> 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 th=
reads 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 w=
e would have lower frequencies of emails, review requests and people contri=
buting, hence I can move around a lot of code (mostly package renames to or=
g.apache.cloudstack in cloud-api) and right now the merge conflicts are rea=
lly 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 s=
trings. All requests done via UUIDs (and not CloudStack's internal db's IDs=
).
>> 1. https://issues.apache.org/jira/browse/CLOUDSTACK-518 Fix @Parameter a=
nnotation to have annotation field to a Response class which would give us =
entities (interface to VO objects). This would get rid of all IdentityMappe=
r using which was used earlier to get VO entities from an annotated table n=
ame. 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 ex=
ist in CloudStack's API server but policy checking is moved out of CloudSta=
ck. (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 sh=
are before the merge.
>> 3. Group APIs to org.apache.cloudstack.api.{command,response}.{entity1,e=
ntity2 etc.} packages. This is mainly done for developers, so when they wor=
k 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 wa=
s mostly file movement to org.apache.cloudstack.api package and helped us t=
rack 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 newes=
t commiter, Min Chen. The aim is to simply response, right now for. example=
 when we listVMs we don't want unnecessary (serialized) response objects wh=
ich could be queried using uuids separately.
>>
>> Pl. ask away any doubts, questions and concerns you may have. It was cha=
llenging 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 pres=
ent 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 pat=
ches 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 functio=
nality 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=3D-1 which should be a stri=
ng 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 i=
ssues.
>
> 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/201=
212.mbox/%3CCAKprHVbdcm1Ot2v%3DH8zCHmHhgX5iS3OyoVgH8Vh7XOmXhmBX5w%40mail.gm=
ail.com%3E