2013/2/23 Felix Meschberger <[email protected]>: > Hi > > I am looking into cutting a Web Console release soon, which will require a > release of the "inventory" bundle first. This is the bundle previously called > status having been renamed because the maven module name "status-printer" was > not congruent with the bundle symbolic name o.a.f.status. > > I have been looking into this module a bit and would like to share some > feedback: Great!
> > * IIUIC an InventoryPrinter registered with a .category property will be > registered with the web console as a proxy plugin with the respective > property (allowing the InventoryPrinter to be accessed by its own URL). If > the .category property is not set, the default Inventory category is used. I > think we should not have such a .category property and these adapter plugins > should always be registered with the Inventory category. Actually the category property is used for two purposes - the Inventory stuff can be used without the web console, that's one of the main reasons to create this separate bundle in the first place. So I think it makes sense to have a categorization of printers when using the inventory API directly (see below). > > * Configurability: Should the inventory- URL prefix and the Inventory > category be configurable ? I think the category, yes - as this is usable outside of the web console - prefix, no > > * Should the ZipAttachementProvider really extend the InventoryPrinter > interface ? I rather think it should not extend the InventoryPrinter > interface. Rather it should be used as a "marker with API": When writing the > ZIP, the InventoryPrinter services are checked whether they also implement > the ZipAttachementProvider and should be called accordingly. The ZipAttachmentProvider is in fact an extension of the InventoryPrinter - so why hide this fact? The implementation always checks all inventory printers whether they implement this interface, so I see no benefit of having this a separate marker interface. > > * Minor: Source Code Formatting should be adapted to our coding conventions. > > * What are the InventoryPrinterHandler and InventoryPrinterManager interfaces > used for ? Looks like they are used internally in the Inventory bundle -- are > they used externally ? I have the impression, this API is not required. They can be used by any client code to get inventory printers - so for example if you want to write a gogo shell extension printing out all information from the inventory printers, these extension can use the printer manager to get these things. I've added the handler interface as this one makes handling the printers easiers by client code and clearly states that client code should never use the printer services directly. Of course one could argue that client code could lookup all printers and use them directly, however this would duplicate all the logic we already have implemented and therefore I came up with the manager/handler API for this. > > * API: We should probably tag the interfaces with the BND @ConsumerType and > @ProviderType annotations. Yepp > > * While I think "Inventory" as a name is probably better (and less confusing) > than Configuration Status or Status or Status Printer, I am not really > convinced (and still fear the name is confusing). Maybe some native english > speaker has a better name for that ? Yepp good idea - I never liked the status/status printer name and after some searching came across "inventory" as a name as this is nearly used anywhere, so we avoid naming confusing with existing stuff. But then, I'm not a native speaker either. Carsten > > Regards > Felix > > PS: Releasing the Inventory bundle blocks the Web Console release because the > "all-in-one" profile of the Web Console embeds the Inventory bundle. -- Carsten Ziegeler [email protected]
