brooklyncentral/camp-server — pull request #1 by ahgittin (2013-05-23 16:24:23+00:00)
Title: Tidy, addressing comments from Aled on initial check-in

(Aled's comments in blockquote.) Thanks -- very helpful and nearly all resolved.

Two points where I'd especially like feedback are marked with XXX below.

> Overall it looks good.

> PlatformComponentTemplateBrooklynLookup:
> Are all the brooklyn catalog items really platform component templates, rather than some being application component templates?
> Many are blueprints for starting applications.
> But I guess you can view that app as a "solution" offered by the platform, such as MapR etc?

indeed. Brooklyn doesn't represent ACT's -- which are application artifacts e.g. WAR files -- instead we use config parameters which point to these.

> Do we need to do caching of PlatformComponentTemplate etc?
> Suggest we continue to do without caching for now.

I think a usual impl will be that they all come from a database. I have deferred caching and entitlements but tried to make it such that these are straightforward to add.

> camp-base io.brooklyn.camp:
> perhaps should separate some content into a separate spi package?

moved .impl to .spi -- seems right

> The cross-references between packages is a bit strange, e.g.
> camp.util.collection.ResolvableLink extends Link in camp.impl
> camp.CampPlatform references AbstractResourceListProvider in | camp.util.collections

move .util.collection to .spi.collection -- also seems right

> Could we have a clean separation of interfaces and spi?

XXX - i am sympathetic to this idea on moral grounds but pragmatically i don't think it's worth the work (and the inevitable naming mess), at least not yet -- do you feel strongly otherwise?

> References to (abstract) classes have leaked out
> e.g. AbstractCampRestResource takes a AbstractResourceListProvider rather than a ResourceListProvider

yep, done

> CampPlatform:
> Does it offer enough functionality to justify the AbstractClass rather than an interface?
> Could then have just BasicCampPlatform for extending.

let's keep an eye on this but i think there may be more root-level behaviour in time; prefer not to extend BasicCamp as i expect it is only suitable for noddy implementations -- anything real would have to supply ResourceLookups

> CampPlatform.initializeRoot:
> bad idea to override this when it's called by the constructor of CampPlatform. The fields in the overriding class won't have been initialized etc.

good spot, fixed

> could overload CampPlatform constructor to take a PlatformRootSummary instead

done, much nicer. more reason to question the desire CampPlatform being abstract but i do suspect there will be more wanted there.

> ResourceListProvider
> First reaction was that it feels like a lot of stuff for a provider of a list.
> But seeing more how it's used, not sure how best (if at all) to simplify it).

... and i expect there will be more -- supporting paged/cursor-based sublists, context (e.g. things a user is allowed to see, filtering on tags) -- which a DB-backed impl would be able to optimise

> Also, ListProvider the best name? If it were just a list provider, then guava's provider and a java.util.List would have sufficed.
> It looks like a list of Link instances, and a cut-down map keyed off the id.

yeah. changed to "Lookup".

> For serialization/deserialization, is it just the DTOs (so nothing in camp-base)?
> i.e. are we free to use final in all other places for which it makes sense?

yes -- e.g. in Link. but in many places (see eg BasicResource) there are many fields so an inner class public builder with setting private vars is easier to use than a big constructor imho; fields are effectively final (set before the builder returns the object and immutable after) but not technically final

> ResourceDto.newInstanceInitialization
> Wondering about best pattern; is overloading the constructor (i.e. also having a no-arg for json) a more natural way of doing this?

XXX - on the fence. i've done it, in its own commit -- interested in your feedback. code slightly more concise, feels sensible from java perspective but slightly jarring in terms of "both bean and non-bean constructor"

> CampRestUtils
> Not convinced by name: providing access to the underlying CampPlatform suggests its more of a context than a utils.

yep, done -- class and accessing method+field renamed

> Should we fail-fast if CampPlatform or DtoFactory is not supplied?

yep, done

> PlatformComponentTemplateBrooklynLookup
> I'd prefer to pass in ManagementContext and PlatformRootSummary, rather than leaking reference to BrooklynCampPlatform in its constructor

yep, done

> PlatformComponentTemplateBrooklynLookup.links:
> Can use super.newLink(...)

yep, done

> Use of Preconditions.checkNotNull:
> Don't need to do your own Strings.format, such as:
> checkNotNull(blah, Strings.format("No @Path on type %s", restResourceClass))
> Can do:
> checkNotNull(blah, "No @Path on type %s", restResourceClass)

yep, done (looked for that but didn't see it so thanks)

> AbstractCampRestResource:
> Presumably mention of ManagementContext is a copy-paste error from brooklyn code?

yep, done