Hi Eoghan,
Thanks for the review
On Wed, 2010-05-05 at 20:55 +0100, Eoghan Glynn wrote:
Hi Mark,
Just went through the storage domains/attachments commits and generally
it all looks good.
A few points below, some of which may be flowing from a misunderstanding
on my part around the cardinality of the attachment relationship.
Cheers,
Eoghan
api/src/main/resources/api.xsd:
+ <!-- rel="self" is a reference back to the resource itself -->
+ <!-- other resource specific link relation types may be definied
-->
+ <xs:element ref="link" maxOccurs="unbounded">"
Should we specify minOccurs="1", as a self link is a minimum
requirement?
Good point, but I want to move from:
<vm>
<link rel="self" href="..."/>
...
</vm>
to:
<vm href="..."/>
...
</vm>
See:
https://fedoraproject.org/wiki/Cloud_APIs_REST_Style_Guide#Links
but I guess your point still stands, except it should be minOccurs=0 ?
(I went further with Actions and specified a minOccurs of 2, self
and
replay)
Yep, saw that
Something didn't sit right with me about that, and now I realize what it
was - doesn't the client do this:
POST /vms/1234/reboot
<action>
<async>true</async>
</action>
i.e. having no <link> elements is valid, right?
--
"Looks like:
<attachment href="/storagedomains/1234/attachments/1111">
<status>ACTIVE</status>
<storage_domain>
<id>1234</id>
<link rel="self" href="/storagedomains/1234"/>
</storage_domain>
<datacenter>
<id>4567</id>
<link rel="self" href="/datacenters/4567"/>
</datacenter>
</attachment>"
Do we need to embed a full datacenter and storagedomain element in the
attachment, as opposed to just a link to each?
It's not a full datacenter, just a reference that includes both the URI
and ID. I think it'll be more clear when both of those are attributes.
Also I thought each attachment represented the 1:1 relationship
between
a particular data centre and an individual storage domain.
Yep
But the schema definition of Attachment:
<xs:element name="datacenter" type="DataCenter">
...
<xs:element name="storage_domain" type="StorageDomain"
minOccurs="0"/>
<xs:element name="status" type="StorageDomainStatus"
minOccurs="0"/>
with exactly one datacentre, but zero or more storage domains and
statuses, suggests that it represents the 1:N relationship between a
datacentre and all its associated domains. Is that correct?
If not, probably best to change the schema to make the one-to-one-ness
of each attachment explicit.
But if it really is supposed to be 1:N, then if there are multiple
storage domains in an attachment, is there a cardinality-matching and
ordering issue with the corresponding statuses?
Good point - I'm just being lame about schemas
The rules I'm trying to express are:
- A client may POST an <attachment> containing only a <datacenter>
reference. The storage domain is implicit
- The server will always respond with an <attachment> containing both
a <datacenter> and <storage_domain> reference
Also am I correct in assuming that we use the same attachment
representation when navigating from the data centre end of things (e.g.
GET /datacenters/4567/attachments) as opposed to the storage domain end
(e.g. GET /storagedomain/1234/attachments)? Because half the information
could be implicit in each case, e.g. the storage_domain element in every
attachment would be /storagedomains/1234 if we started
from /storagedomain/1234/attachments.
Right, the storage_domain is implicit, but given a URI for an
attachment, we still need a link to allow the client to get back to the
storage domain again
As for listing attachments under a datacenter, since it's read-only,
I've just done:
<datacenter>
...
<attachments>
<attachment href="..."/>
<attachment href="..."/>
<attachment href="..."/>
</attachments>
</datacenter>
Which sucks quite a bit - I guess we could include the entire attachment
representation?
--
api/src/main/java/com/redhat/rhevm/api/resource/AttachmentResource.java:
+ @POST
+ @Actionable
+ @Path("activate")
+ public void activate();
+
+ @POST
+ @Actionable
+ @Path("deactivate")
+ public void deactivate();
We'll have to do an audit thru' the actions on the various resources and
decide which need the async treatment. Maybe all of them do, for
consistency. Maybe only those of potentially long duration.
Yep, agree - we need to review them all
--
Nit-pick:
api/src/main/java/com/redhat/rhevm/api/resource/AttachmentResource.java
had a couple spurious imports.
Actually there were a few of those around in the source tree so I went
ahead cleaned them up and pushed as I had eclipse open with its
pernickety warnings.
Great, I had intended to do a sweep of them soon :-)
Cheers,
Mark.