-
1. Re: Abstract Beans JBKERNEL-10
dmlloyd Jul 27, 2009 3:51 PM (in response to marius.bogoevici)"marius.bogoevici" wrote:
One issue I encountered is that metadata such as PropertyMetaData is stored in a Set in the BeanMetaData, yet PropertyMetaData does not implement equals() and hashCode().
This leads to the possibility of writing definitions such as:
I ran into this as well. To add to the strangeness, the AbstractBeanMetaData.getProperty(String) method iterates the whole set looking for a matching property, incurring a O(n) overhead. BeanMetaDataBuilder does much the same (in this case, iterating the whole set to remove the duplicate-named property).
In my opinion, the backing structure (in both BMD and BMDB) should be changed to a LinkedHashMap<String,PropertyMetaData> ensuring that properties are installed uniquely and in order. -
2. Re: Abstract Beans JBKERNEL-10
marius.bogoevici Jul 30, 2009 11:23 AM (in response to marius.bogoevici)Since parsing is done using JAXB, I think we still need to use a Collection (Set is alright, although it does not take advantage of any uniqueness feature of PropertyMetaData). Maps would be mapping somehow differently in JAXB, and AFAICT you can't actually use a HashMap per se as the mapped property, much less so a LinkedHashMap.
I would rather think of using a set for JAXB and a map internally, something like this:... @XmlTransient private Map<Object, PropertyMetaData> propertyMetaData = ; public void setPropertyMetaData(Set<PropertyMetaData> parsedProperties) { this.propertyMetaData = new LinkedHashMap(); <add all the elements of parsedProperties into the Map> } @XmlElement("property") public Set<PropertyMetaData> getPropertyMetaData() { return new HashSet<PropertyMetaData>(this.propertyMetaData.values(); }
-
3. Re: Abstract Beans JBKERNEL-10
dmlloyd Jul 30, 2009 11:40 AM (in response to marius.bogoevici)Yeah, that was my thinking as well. There is a lesson to take away from this however: defining your API object model around JAXB makes for ugliness and bad things unless you're extraordinarily careful and clever. :-)
-
4. Re: Abstract Beans JBKERNEL-10
alesj Jul 31, 2009 11:24 AM (in response to marius.bogoevici)Some thoughts about this approach.
I wouldn't put abstract definitions into Controller (under Descibed),
as I think they would do more harm than good on the overall impact.
I would keep them in some nice, useful registry,
where I would also be able to define proper dependencies on them.
e.g. real bean deployed before its abstract parent, would mean dependency item on this abstract-registry
This should be an impl detail of AbstractKernelController,
where the real abstract handling code would be separated.
Or why would you want to keep it as part of Controller?
Wrt Properties.
I guess both of your use cases proved we need better impl.
Kabir, can you please have a look at what we can do there? -
5. Re: Abstract Beans JBKERNEL-10
marius.bogoevici Aug 2, 2009 1:40 PM (in response to marius.bogoevici)"alesj" wrote:
Or why would you want to keep it as part of Controller?
I guess, I should answer this first. Initially, I tried to use BeanMetaData directly, but the problem I've been facing was that I found no means to retrieve the BeanMetaData directly, other than through the ContextController that was created through it.
To me, we have 2 different problems that we have to solve: abstract beans and parent-child inheritance. They are different, because parent-child inheritance should be possible even if the parent is not abstract.
Also, I considered the possibility that the parent and child are to be found in different deployments, even different scopes (with the parent being in the broader scope, obviously).
Also, one needs to be consider that the parent can be undeployed or redeployed (what happens in this case? I would think that if we associate abstract beans with controller contexts, we could have dependencies and undeploy the child beans automatically).
So, overall, the reason why I think this could be done this way is because I found out that the metadata management, as it is currently implemented, is based on the Controller/ControllerContext mechanism, once the BeanMetaData has been parsed and deployed by the Deployer all the references to it are held by ControllerContexts and managed via the Controller. This means that for getting the metadata for a particular bean, I have to go to the ControllerContext that has been created for that metadata. Of course, we can have better separation between metadata and the actual ControllerContext instances by introducing a registry holding the MetaData prior to deployment, but I think that this should also come at a price of a refactoring, to avoid duplicating features, such as scoping, for example, so I am not to sure how justified it is from the effort point of view. Of course, you know the MC much better inside out, so I'd like to know what your thoughts are about this."alesj" wrote:
I wouldn't put abstract definitions into Controller (under Described),
as I think they would do more harm than good on the overall impact.
Do you have in mind anything specific or is this a general statement?
Since there is a clear distinction between "DESCRIBED" and "INSTANTIATED" and "INSTALLED", I thought that it would be clear that the ControllerContext is not been backed by an actual instance, and this has the advantage that we can use all the existing mechanisms for management, scoping etc. But I'm interested to hear more on the topic."alesj" wrote:
I would keep them in some nice, useful registry,
where I would also be able to define proper dependencies on them.
e.g. real bean deployed before its abstract parent, would mean dependency item on this abstract-registry
This should be an impl detail of AbstractKernelController,
where the real abstract handling code would be separated.
See above. If we were to have a definition registry, I think it should hold all the definitions (abstract and non-abstract) and it should be aware of scoping. But now that you're mentioning dependencies, I think it would be more natural to have child A depend on parent B in state DESCRIBED, rather than on a generic registry. With the former, child A cannot be installed until the abstract parent B is DESCRIBED, whereas if we had a single abstract-registry, that dependency would always be satisfied, even if the definition for the abstract bean hasn't been deployed yet, so I can't see what would be the use. Also, parent-child relationships can exist even between concrete beans, so if the registry is holding just abstract beans it would be of little use. -
6. Re: Abstract Beans JBKERNEL-10
alesj Aug 2, 2009 4:18 PM (in response to marius.bogoevici)Actually thinking more about this and reading your response,
it makes sense the way you did it - by having abstract beans in Described.
As this already address some of the issues;
e.g. scoping (PreInstall), CL existence (Described)
But we should then do the following things:
* have new ControllerContextActions for abstract beans, which would only have PreInstall and Describe mappings
* requiredState is Descibed
* is not autowire candidate
Or any other optimization that could have impact on the issues mentioned here:
- http://www.jboss.org/index.html?module=bb&op=viewtopic&t=158498
altough I doubt having a few abstract beans would do much harm.
I'll check your patch asap, or if you're not addressing some of the issues
I mentioned above, I guess you can do a new one, and I'll have a look then. :-) -
7. Re: Abstract Beans JBKERNEL-10
marius.bogoevici Aug 3, 2009 11:30 PM (in response to marius.bogoevici)Cool!
Let's wait for a couple of days until I produce a new patch. I'd rather do some code cleanup first. -
8. Re: Abstract Beans JBKERNEL-10
kabirkhan Aug 11, 2009 11:30 AM (in response to marius.bogoevici)"marius.bogoevici" wrote:
@XmlElement("property") public Set<PropertyMetaData> getPropertyMetaData() { return new HashSet<PropertyMetaData>(this.propertyMetaData.values(); }
I have taken a look at this, and one problem is that some places in the code rely on the properties set being mutable, so returning a new set breaks existing things.
We need to either:
- document the immutability in the SPI and change the known uses (probably not advisable)
- or I need to somehow use both a map and a set wrapped together somehow so that they are notified of each others changes
I'll take a look at the second option -
9. Re: Abstract Beans JBKERNEL-10
kabirkhan Aug 11, 2009 5:14 PM (in response to marius.bogoevici)The problem in doing this refactoring is that BeanMetaDataBuilder (which can be changed) and JBoss XB (which cannot be changed) do something like:
BeanMetaData bmd = ...; HashSet<PropertyMetaData> properties = bmd.getProperties(); if (properties == null) { properties = new HashSet<PropertyMetaData>(); bmd.setProperties(properties); } properties.add(propertyMetaData1); properties.add(propertyMetaData2);
So if we copy the set into a map when calling BMD.setProperties() the properties.add() calls do not get reflected in the underlying data in the map. I have looked into writing a Map enabled wrapper around the passed in set, but since we will need to work with the set underneath there does not seem to be that much point. -
10. Re: Abstract Beans JBKERNEL-10
alesj Aug 13, 2009 6:51 AM (in response to marius.bogoevici)I don't think the map really solves all the problems.
Since afair we have some beans that have properties with the same name but different types.
Dunno how much against JavaBeans is this, but I think we should still be able to support is - which is somehow harder to do with map.
What about just some useful merging methods on (Abstract)BeanMetaData? -
11. Re: Abstract Beans JBKERNEL-10
kabirkhan Aug 13, 2009 8:53 AM (in response to marius.bogoevici)"alesj" wrote:
I don't think the map really solves all the problems.
Since afair we have some beans that have properties with the same name but different types.
They key could be name+type
Anyway, I don't see a way to make it a map without changing the spi