osgi / bugzilla-archive

Archive of OSGi Alliance Specification Bugzilla bugs. The Specification Bugzilla system was decommissioned with the move to GitHub. The issues in this repository are imported from the Specification Bugzilla system for archival purposes.
0 stars 1 forks source link

[blueprint] Should blueprint config admin integration use ${propName} syntax #1606

Closed bjhargrave closed 12 years ago

bjhargrave commented 14 years ago

Original bug ID: BZ#1738 From: Alasdair Nottingham <not@uk.ibm.com> Reported version: R4 V4.3

bjhargrave commented 14 years ago

Comment author: Alasdair Nottingham <not@uk.ibm.com>

RFC 124 had the concept of a property placeholder. This is not currently part of RFC 156.

We should discuss and come to a consensus on the following:

  1. Should property placeholder support be included in RFC 156.
  2. If property placeholder support is to be included should we better integrate it with the dynamic support.

This has been discussed before via email lists and in face to face.

The most recent email discussion was triggered by this email: https://mail.osgi.org/pipermail/eeg/2010-August/006857.html

bjhargrave commented 14 years ago

Comment author: Glyn Normington <gnormington@vmware.com>

Thanks for raising this bug. It is important to reach a concensus to avoid a rework cycle later.

The rationale for not introducing property placeholders into blueprint is as follows:

  1. They are not necessary. The current technical solution is functional and usable without them.

  2. Unless we complicate the blueprint model by introducing some kind of XML file scope or new scoping syntax, property placeholders are global. Since they do not refer to the XML element which introduced them, they are unstructured and can easily cause confusion for users.

  3. Property placeholders, as defined by Spring, are general in nature and not tied to Config Admin, so they would complicate (see below for some of the complications) the base blueprint spec, implementation, and TCK rather than being a sub-feature of the Config Admin support.

  4. The ${} syntax needs careful definition in terms of its exact syntax, where in XML it is allowed, and how it is escaped (if the user needs to use ${} in a string).

  5. Property placeholders need to be defined and then resolved in additional lifecycle phases which considerably complicates the blueprint lifecycle model.

  6. Property placeholders cannot be implemented using a RFC 155 namespace handler, so they are much less modular than the current technical solution (which can). Modular specs and implementations are a key way of controlling the increasing complexity of OSGi.

  7. There was discussion of property placeholder support in RFC 124, but this was dropped for good reasons. Property placeholder support implies breaking down each declaration into a generic bean definition. There was a big discussion on whether the idea that everything is a bean should be merged across, from Spring to blueprint, and the decision was taken not do this (which complicated some other aspects of RFC 124). Thus blueprint has managers, each with its own properties, lifecycle, and functionality rather than the unified model of Spring. Retrofitting property placeholders into this model would be very difficult. Changing the model to be more Spring-like is not an option as it would break backward compatibility.

  8. Using a limited form of property placeholder notation for Config Admin was also explored in RFC 124 but the conclusion there was that the approach could not handle updates.

  9. Property placeholders imply post-processing. In addition to requiring a new lifecycle to indicate when this processing can take place, blueprint would have to define a contract - what can be processed, to what degree and in what order. We would also need a means to define the post processor since, as mentioned in point 7, blueprint does not have a unified model. Would we introduce a new element/type/manager?

  10. Property placeholders tend to be static and are designed for one-off replacement and so are not at all suited to supporting Config Admin updates. Such updates do not fit in with the concept of a post-processor which would need to modify the definitions at runtime - not something that blueprint supports.

  11. Costin looked at what it would take to add property placeholder support into blueprint in order to provide the syntax for use by the Config Admin support and his conclusion was that, although it might sound doable, it really isn't. There are lots of subtleties in Spring's property placeholders today and the dynamics of OSGi only complicate matters more. The necessary changes to the core blueprint container and APIs would be far too large in his opinion.

  12. From the perspective of the specification, I cannot justify introducing property placeholders unless the current technical solution proves to be unworkable or unusable. To ensure clean semantics, I wrote a formal model of the current approach (available alongside the RFC) and the current technical solution is based on that, which means the concepts are known to be well defined even before we prove them via prototyping.

If the above points are still not sufficiently clear, we can organise calls to explain them in detail in order to reach the desired consensus.

bjhargrave commented 14 years ago

Comment author: Alasdair Nottingham <not@uk.ibm.com>

The specific proposal I made is documented in this email:

https://mail.osgi.org/pipermail/eeg/2010-January/005786.html

bjhargrave commented 14 years ago

Comment author: Alasdair Nottingham <not@uk.ibm.com>

Hi,

Glyn, Costin, Graham and I had a very positive telephone conversation about this proposal. This is the summary of the discussion:

There was some confusion over the term property placeholder. The term was used in RFC124, however it also exists in spring and is somewhat more powerful. I was not asking for the Spring property placeholders. As a result of this misunderstanding I have changed the bug title to "Should blueprint config admin integration use ${propName} syntax."

In comment BZ#1 Glyn's points 3, 6, 10, 11, 12 stem from this miscommunication. We did not dwell on points 1, 4, 7, 8 because they were either easy to agree, description of history or fairly minor compared to the other points.

The rationale for not introducing property placeholders into blueprint is as follows:

  1. They are not necessary. The current technical solution is functional and usable without them.

No discussion since it is true, the discussion is really about what is the best way to introduce things

  1. Unless we complicate the blueprint model by introducing some kind of XML file scope or new scoping syntax, property placeholders are global. Since they do not refer to the XML element which introduced them, they are unstructured and can easily cause confusion for users.

This already exists to some extent, and is being introduced and discussed under RFC 164 which Glyn has since been commenting on.

  1. Property placeholders need to be defined and then resolved in additional lifecycle phases which considerably complicates the blueprint lifecycle model.

It was agreed that an additional phase may be required, however I noted that the phone implicitly exists anyway and is probably needed for any config admin integration.

  1. Property placeholders imply post-processing. In addition to requiring a new lifecycle to indicate when this processing can take place, blueprint would have to define a contract - what can be processed, to what degree and in what order. We would also need a means to define the post processor since, as mentioned in point 7, blueprint does not have a unified model. Would we introduce a new element/type/manager?

So does any config admin integration, plus it implies the ability to reinject.

If the above points are still not sufficiently clear, we can organise calls to explain them in detail in order to reach the desired consensus.

Another point made by Glyn was the proposal I made back in January 2010 provides a semantic gap between the config admin properties and the use of ${propName} which makes it less obvious what ties up with what. Also it was noted that the proposal was somewhat magic in that a top level element has effects across xml files. Based on this I make the following amendment to my proposal for consideration:

You can define a default pid for an xml file by placing the default-pid attribute on the blueprint element (like the default-availability property)

When you want to use a different pid for a component you can specify it on the bean property:

so within the bean a the property is resolved in com.foo.aPid and in b it is relative to com.bar.anotherPid.

If I wanted to do managed services I would do something like this:

In this case the mapping from property to value is explicity specified, no auto discovery of properties, but the container will manage updates. So when the prop changes the setter is driven.

Finally for the previous magic container case you can do something like this:

Glyn, please correct me if I misrepresented something, or missed something important and let me know what you think of this updated proposal.

bjhargrave commented 14 years ago

Comment author: Glyn Normington <gnormington@vmware.com>

Feedback on the updated proposal, to be discussed in a F2F meeting Alasdair/Graham/Glyn on 10 Nov:

F1. There is no requirement to define a default PID for a XML file. See the agreed requirements in section 4 of RFC 156.

F2. The property element, a la Spring, is still used in the updated proposal. This is much too general. At the very least, this needs to be a element, but even then it probably needs to be re-spelled to avoid confusing Spring users. Given that it needs to be in the cm namespace, it is equivalent to the current technical solution's osgix:propertyMapping element but with different concrete syntax including the extra dollar and curly braces.

F3. The property element, as defined, seems to have its value substituted at bean construction time. The semantics of a placeholder ${keyInPID} is to deliver the value corresponding to keyInPID and not the key itself. The mapping from PID key to property name is thereby obscured and it is this mapping which needs to be remembered and reapplied when updates happen.

F4. The updated proposal does not address REQ-MULTI-PID-COMP or REQ-DEFAULT-PROPS nor does it define how these requirements are addressed in relation to update.

F5. There is no advantage of the update approach (described as "managed services") in the updated proposal over the current update solution in section 5.5 of RFC 156. The EEG has already agreed that driving setters bracketed by driving optional begin and end methods is the way to handle update. Revisiting that decision and reworking the update solution doesn't appear related to the use of ${propName}.

bjhargrave commented 14 years ago

Comment author: Alasdair Nottingham <not@uk.ibm.com>

After a face to face meeting between Glyn, Graham and Alasdair we have discussed the latest proposal in comment BZ#3 and the feedback in comment BZ#4 and we came up with the following modified proposal:

This says "inject into myProp the value bound to 'KeyInPid' from from pid 'com.bar.pid' if no such key exists inject 'this is the default' instead"

If you wish to use multiple different pids for a bean this can be achieved by placing the cm:pid on the property. e.g.

This addresses points F2, F3, and F4 from Glyn's feedback. F1 will be addressed by raising another bug to discuss (should this proposal gain agreement) whether we should allow a default-pid at the blueprint level. We agreed the current update proposal is right which addressed F5.

We did have a few questions:

  1. what does it mean to have a name attribute, but not value or cm:key? We are not sure what happens if you have a property with no value. This is valid xml, but we didn't know what the spec says, or the implementations do.
  2. How does the namespace handler ensure that it is called to resolve the initial value at bean creation. It was proposed that the namespace handler should replace the property's value at parse time with a extension to the ValueMetadata that might be called CMValueMetdata. When the get method on the value is called it would resolve this from the config admin dictionary. Core blueprint doesn't know about CMValueMetadata, just ValueMetadata so it should work nicely.
bjhargrave commented 12 years ago

Comment author: Tim Diekmann <tdiekman@tibco.com>

Reassigning to EEG inbox and marking for future release.

bjhargrave commented 12 years ago

Comment author: Graham Charters <charters@uk.ibm.com>

Closing because the cm:key/cm:pid design is now in the RFC and has replaced the ${propName} notations.