jclouds / legacy-jclouds-labs

https://jclouds.apache.org
0 stars 18 forks source link

Add jclouds-dto project #24

Closed iocanel closed 11 years ago

iocanel commented 11 years ago

This pull request add jclouds-dto to labs.

This project intends to provide "data transfer objects" for most of the jclouds domain objects. The idea is to have objects that are easily serialized so that they can be used for exposing jcouds to jmx, etc.

The project consists of 2 modules: i) dto-core: Which provides the dtos ii) dto-converters which provides functions for converting the domain objects to dtos.

Note: In some cases I am using an ImutableSet builder, over guavas lazy iterables (e.g. transfomed iterables), mostly because the lazy stuff had some issues with serialization. I am not aware of any better way (an other idea is to create a set manually?).

iocanel commented 11 years ago

A few comments about the management project.

The management project has 3 modules: i) core ii) compute iii) bobstore

The core component adds a modules, which adds a bindListener for View and each time a View is created it checks if the View can be managed. If so, it registers the View to the management context, which then exposes the View to JMX.

The disocery of MBeans for each View is done with by Looking Up for ViewManagementFactory in the serviceloader or osgi service registry, like we already do with ProviderMetadata and ApiMetadata.

The MBean are using the dto converters which are added in the first commit of this Pull Request.

The compute and blobstore modules container the view management factories and mbeans for compute and blobstore.

demobox commented 11 years ago

Couple of quick questions - apologies if these have already been discussed on IRC:

iocanel commented 11 years ago

The reason for creating the jclouds-dto project as a separate project, is that there will be possibly other parts that would use that (e.g. byon already uses something similar to dtos. Also, it would help other parts that might need serialization etc).

I've experimented with reflection for converting domain objects to opentypes in my first pull request. My experience was that it was too much work. Since we target a very small set of objects (that don't change that often), implementing a really generic approach doesn't worth the effort.

Now, regarding libraries we could use... Bean-utils could be one solution. I am not sure how well it will behave with lazy guava collections, or types like optional etc. But does it worth introducing dependencies for such a trivial task?

demobox commented 11 years ago

But does it worth introducing dependencies for such a trivial task?

Not if we regard it as trivial, no ;-) And if we don't see much risk of bit rot due to changing types that's probably a double "no" ;-)

codefromthecrypt commented 11 years ago

I'm certainly not interested in making a dep for the copying back/forth. Current approach is isolated, testable, and would only result in some order of kilobytes less than 100.

abayer commented 11 years ago

Yeah, I like the idea of reworking byon - that's been on my when-I-get-a-chance list for a while now, so I'd like jclouds-dto to be a separate artifact to make it easier to pull in elsewhere in the future. And yeah, no real need for more dependencies, IMO.

demobox commented 11 years ago

Current approach is ... testable

Speaking of testing, what kind of level of unit testing is needed here? I see that e.g. ToApiMetadataTest only checks that one attribute is copied correctly.

More complicated tests for straight attribute copying seem a bit like overkill, but it would be good to have a way of guarding against missing an attribute in the DTO if one is introduced in the base object.

codefromthecrypt commented 11 years ago

I've recently gone through a process in denominator very similar to this. Basically, we need to be able to serialize, deserialize, and facilitate representations that are often maps or json. If you look at the RData section of this wiki, you can see a plan that makes all dto stuff implement Map<String, Object>.

If you look at the code, you'll see a validation strategy and also how constructors are param tagged to facilitate immutable plugins to jackson or gson.

This style leads to easy use within collections, where you are sending or receiving a bunch of things within an envelope. ex. RestfulList<BlobMetadata> which could be stored in any system as a list of maps.

Not saying we need to emulate this approach in jclouds, just food for thought, as I've recently spent a month or 2 on the issue in another project.

codefromthecrypt commented 11 years ago

I'd also take care to ensure the dto objects are as flat as possible. This implies ridding of unnecessary relational data, especially ones that recurse (like node.location -> node.locationId). Assume the store this stuff is going to have a peer collection, so any overlapping data can be referenced by id is my take.

codefromthecrypt commented 11 years ago

I think there are some strategies in guava tests wrt testing copying etc. at the very least, we should have a bunch of well-known objects that are tested by deep comparison and things like equals, hashCode, and toString. A testng @DataProvider could be used for this.

codefromthecrypt commented 11 years ago

last big comment. this project should take into consideration existing things that wrap jclouds data, such as the types in whirr, jenkins, pallet, and byon. Generally, these are flatter structures. Let's consider how this can do the ecosystem (and ourselves) greater good than just adding serializable.

codefromthecrypt commented 11 years ago

I'd format all the dtos per above advice and squash them into the same commit. converters + management are code that belong in corresponding modules in core, blobstore, or compute. make a separate commit for each of these in the packages they will end up in post-labs (ex. codec for blobstore in org.jclouds.blobstore.X)

codefromthecrypt commented 11 years ago

and please someone suggest a word besides dto. It feels like it should be in an old "java core enterprise practices" book along with some bad advice.

I'm partial to "representation" (as in REST) as this is in fact what it will be used for, but maybe others like the bike shed in green with a blue border?

demobox commented 11 years ago

Before we get into the colour of the bike shed...are you talking about a package name here or some kind of suffix for each class? The current suggestion seems to be to use the same names for the classes as for their representations, which is elegant but potentially a pain to work with in IDEs, since you have to look at the package to figure out exactly what you have.

And you need fully-qualified names to use both in one scope, although admittedly only the converters here should be in that position, and there it makes sense anyway.

codefromthecrypt commented 11 years ago

no I'm talking about the package and maven artifact name.

Wrt the other issue, we already have similar issues in the IDE wrt classes like Provider, static inner classes named Status, etc. Class naming to be friendly is an optimization which only sacrifices conversion functions (aka codecs), which I think is a better place to take the punch vs slapping Representation or DTO or anything else on class names.

wrt packages, we probably should concede that these representations will need a version-specific package similar to what we have in rest apis.

ex. org.jclouds.representation.blobstore.v1.Container

On Sat, Apr 6, 2013 at 4:24 PM, Andrew Phillips notifications@github.comwrote:

Before we get into the colour of the bike shed...are you talking about a package\ name here or some kind of suffix for each class? The current suggestion seems to be to use the same names for the classes as for their representations, which is elegant but potentially a pain to work with in IDEs, since you have to look at the package to figure out exactly what you have.

And you need fully-qualified names to use both in one scope, although admittedly only the converters here should be in that position, and there it makes sense anyway.

— Reply to this email directly or view it on GitHubhttps://github.com/jclouds/jclouds-labs/pull/24#issuecomment-16006020 .

demobox commented 11 years ago

no I'm talking about the package and maven artifact name

Aha, thanks for clarifying. I'd also prefer "representation" over "dto" but then the domain objects are also a "representation" of some kind, if you ask me.

The thing I'd like to capture is what is special about this representation? Is it a "wire representation"? A "flat representation"? A "transferable/serializable representation"? Can we come up with some good attribute to describe them?

ex. org.jclouds.representation.blobstore.v1.Container

Ah, interesting. Any thoughts about how the versioning for this would relate to the versions of jclouds themselves?

codefromthecrypt commented 11 years ago

flatter representation optimized for humans that write code or expose jclouds as restful resources, and don't use or want the nested relationships in the current model or how they serialize in map, json, or yaml form.

ex. https://github.com/proofpoint/cloud-management/blob/master/src/main/java/com/proofpoint/cloudmanagement/service/InstancesResource.java ex. https://github.com/jclouds/jclouds/tree/master/apis/byon ex. https://github.com/pallet/pallet/blob/develop/src/pallet/compute/node_list.clj

wrt versioning. We need a version of the representations. I'm not going to guess when this will change, but we know it will.

demobox commented 11 years ago

flatter representation optimized for humans that write code or expose

org.jclouds...flat? org.jclouds...readable?

I'm not going to guess when this will change, but we know it will.

grin. OK, so v1 to start with and we'll take it from there, I guess... ;-)

demobox commented 11 years ago

org.jclouds.flatmodel?

codefromthecrypt commented 11 years ago

lets take a poll of similar projects and how things are named. for example, there must be some sanely designed restful service implemented in some packaging structure where the language-specific namespace is differently named than the api namespace without resorting to weird names.

codefromthecrypt commented 11 years ago

not saying it is sane or not, but the rest api for cloudstack is in fact in the "api" package https://github.com/apache/incubator-cloudstack/blob/master/api/src/org/apache/cloudstack/api/response/HypervisorResponse.java

codefromthecrypt commented 11 years ago

nova is in a "views" package https://github.com/openstack/nova/blob/master/nova/api/openstack/compute/views/servers.py

codefromthecrypt commented 11 years ago

is the type ViewManagement used anywhere? I might nix it as it is a slightly confusing term.

iocanel commented 11 years ago

Yes, the view management is used in the management lifecycle module, so that it creates and generetes an mbean for the view whenever a new view is bound.

So ViewMBean maybe?

codefromthecrypt commented 11 years ago

+1 sounds more precise

iocanel commented 11 years ago

Well there is something JMX specific to the BlobStoreManagmenetMBean and that's the MBean suffix. As far as I know this is a required convention for mbean.

codefromthecrypt commented 11 years ago

if it is only the suffix, maybe the management package could extend it?

iocanel commented 11 years ago

I'll try and see how this works.

demobox commented 11 years ago

nova is in a "views" package

I like view or views as it pretty clearly indicates the purpose of these objects. Just wondering if we risk confusing people with the existing View concept?

codefromthecrypt commented 11 years ago

I'd probably take out OperatingSystem and Hardware from NodeMetadata as they are rarely set or correct

zack-shoylev commented 11 years ago

I wanted to go back a bit and ask about reflection again. Is it feasible to use something like javassist to generate the dtos at runtime (I see beanutils was mentioned)? Or is it too much effort still.

iocanel commented 11 years ago

@zack-shoylev: I have no experience with javasist and I cannot evaluate feasibility or required effort, but it sounds like quite a bit of work.

Some benefits of the current approach is that the dtos/representations have no dependency to any jclouds module and can be easily distributed to a third party to communicate with jclouds via jmx (without getting into a jar hell).

Moreover, in cases where it will be used for rendering things into json, having direct control over the format also is a big plus.

zack-shoylev commented 11 years ago

@iocanel : By using a bytecode generation library, the benefits should (?) remain the same. Effort would be moved from long-term maintenance/support to work that would need to be done now. The problem is calculating if this trade-off is worth it. If the represented types do not change much or at all, the current approach will be better - and that seems to be the case.

codefromthecrypt commented 11 years ago

well, the abstraction model seldom changes, so maybe not here.

However, I think there are places where we can consider tooling. For example, our domain objects could have getters|builders crafted with a javac plugin. Possibly other things, too. it might be interesting to convert https://github.com/aplowe/jclouds-bean-cleaner into something similar to dagger which enforces compile-time generation. way off topic, I admit.

On Thu, Apr 11, 2013 at 7:49 AM, Zack Shoylev notifications@github.comwrote:

@iocanel https://github.com/iocanel : By using a bytecode generation library, the benefits should (?) remain the same. Effort would be moved from long-term maintenance/support to work that would need to be done now. The problem is calculating if this trade-off is worth it. If the represented types do not change much or at all, the current approach will be better - and that seems to be the case.

— Reply to this email directly or view it on GitHubhttps://github.com/jclouds/jclouds-labs/pull/24#issuecomment-16239243 .

codefromthecrypt commented 11 years ago

So, I think certain things need to be flattened as noted above, and also ApiMetadata, for example. In representations, we should expose 2 collections ProviderMetadata and ApiMetadata. Both of these would be objects registered in services directory and flatten details (and particularly don't have references to type tokens).

It would probably help for you to put it a "toJson" and "fromJson" test for these. You'll see how serializing deeply nested stuff is a turnoff.

Once this is flattened and weeded out, the next step is to apply it to the existing byon use case. Compare the resulting model and see if it is as easy. When all this is done, I'd actually open the PR against jclouds/jclouds as there you can remove the old "Node" classes and replace the usage with this. Also, you can keep the view-centric classes in the appropriate module (such as blobstore)

make sense?

iocanel commented 11 years ago

I'll update the pr with json tests, so that we can all get a feel if how the generated json would look like.

iocanel commented 11 years ago

Added toJson and fromJson tests, for the representation objects. Made objects more flat.

Should I also flatten LoginCredentials too?

@adriancole You also suggested for some cases filtering? Should the filtering be part of the representation or the converter class?

buildhive commented 11 years ago

Adrian Cole » jclouds-labs #72 SUCCESS This pull request looks good (what's this?)

buildhive commented 11 years ago

Adrian Cole » jclouds-labs #74 SUCCESS This pull request looks good (what's this?)

buildhive commented 11 years ago

Adrian Cole » jclouds-labs #75 SUCCESS This pull request looks good (what's this?)

codefromthecrypt commented 11 years ago

+1

I think this change has had enough review and is a good step forward. Until it is out we can't get much better feedback and stalling hawtio doesn't really help our cause any. I'm sure there will be changes, but these are best done as revisions to the code we've reviewed vs a perpetual branch.

codefromthecrypt commented 11 years ago

@iocanel can you take on making a README or a doc on jclouds.org with the design and integration of this? It is a big improvement.

buildhive commented 11 years ago

Adrian Cole » jclouds-labs #134 SUCCESS This pull request looks good (what's this?)