[jhalliday]

I think it's about time for another round...

Many of the remaining static initializers are concerned with taking a property value from an EnvironmentBean and using it to instantiate a class or classes e.g.

String checkedActionFactory = arjPropertyManager.getCoordinatorEnvironmentBean().getCheckedActionFactory();
Class factory = Thread.currentThread().getContextClassLoader().loadClass(checkedActionFactory);

CheckedActionFactory _checkedActionFactory = (CheckedActionFactory) factory.newInstance();

I'd like something that gives better control over

  a) dynamic changes - it should not be necessary to reload the class (which basically means restarting the server) to change the effective value. Actually this is not just an issue for Class type properties and should probably be discussed separately.

  b) type safety - ideally we'd detect problems at the point setCheckedActionFactory was called

  c) class loading, particularly with regard to availability of the class and supply of any parameters it may need.

I'm mulling over a few alternatives, such as adding a typed version of the getter to the EnvironmentBean

CheckedActionFactory _checkedActionFactory = arjPropertyManager.getCoordinatorEnvironmentBean().getCheckedActionFactoryInstance();

and having some form of update synchronization between that and the setCheckedActionFactory(String) method. That may be sync on write i.e. instantiate the class at the point its stringified name is supplied, or it may be lazy. It may also be desirable to add a typed setter to the EnvironmentBean:

public class CoordinatorEnvironmentBean {

  public void setCheckedActionFactoryInstance(CheckedActionFactory instance) {...}

although then we have also to tackle the reverse update of the String value from the class. However, it would allow for better beans wiring by DI frameworks compared to the current situation. In particular, the CheckedActionFactory impl could require ctor params and have those wired by the DI farmework, which is not possible with the current loading mechanism.

Anybody care to venture an opinion before I pick an implementation alternative at random?

[marklittle]

I was looking at this recently because of some of the unit tests I was writing (wanted to change the setting in one test and then revert in another, but there was no way to revert once it was set). My solution (not implemented) was going to be similar to what you've outlined: hide the synchronization in the bean through a factory. Those classes that really don't ever want to be informed of changes can then implement whatever policy they want at a higher level, e.g., assigning a local variable and not going back to the bean beyond the first call.

[jhalliday]

The problem of dynamically changing property values at point of use is actually orthogonal to the one of holding the same property in multiple forms. Each is a synchronization problem, but of different forms.

For any property, not just strings/classes, there is the question: when does a change to the value in its canonical home (the EnvironmentBean) take effect in things that use that property? The answer right now is: probably never. The value is read from the bean at startup (or first use) and effectively cached at the point of use, usually in a static variable. Therefore subsequent updates modify the bean but not the point of use and are thus ineffective.

For properties that have dual representation e.g. String vs. Class, there is the question of how/when updates to one of those representations affect the other. That's a much more limited problem. Right now for example, putting a getName/getInstance pair on an EnvironmentBean would just require figuring out the synchronization between them and moving the classloading code from point of use to inside the bean. But it won't address the problem that the point of use is calling either getName or getInstance once only and not rechecking either for later updates.

Initially I'm just looking to reduce the duplication of classloading code and allow for bean injection by putting Class getters/setters on the beans. After that I'll worry about the dynamic change at point of use problem. The former has a generic solution for all String/Class cases, whereas the latter will require individual examination of each point of use. In many cases I think we can probably move from static variable to instance variable and static initializer block to constructor for initializing it from the bean. But it's related to lifetime and use of the object constructed.

For example, the useAlternativeOrdering applies in comparison of records, so changing that will cause really screwy behaviour if it leads to comparing a set of records some of which were created before and some after the change. In another case the port number for a listener would require restarting that listener to take effect, which a) is not feasible at present and b) would have consequence for any clients trying to connect if they don't see the update at the same time. That's a distributed form of a problem that also exists inside the same VM - how to coordinate updates where there is more than one point of use for the same property and they need to be consistent. Should a component restart be triggered from the bean, requiring the bean to have knowledge of its users, or do we need an event registration mechanism so the listener can get a callback and restart itself when the bean property is updated? As you an see there are some common themes, each property's case is different and will need thinking about.

[marklittle]

I think you misunderstood me (or maybe I'm still misunderstanding you). I was suggesting that we change all of our classes to not cache the return value from the bean and always call the bean. If other people want to code differently then we won't be able to prevent them caching and hence missing updates. But that's their choice.

The examples you use, e.g., useAlternativeOrdering, could perhaps be treated as read-only attributes (can only ever be set once) versus read-write, and still hidden by the bean. For instance, if there's no setter defined then it has to be one that can only be defined at start-up and not programmatically or when the property file changes (so changes to those properties would be silently ignored, or perhaps given a warning message that they are being ignored.)

[jhalliday]

> I was suggesting that we change all of our classes to not cache the return value from the bean and always call the bean

Indeed. I'm likewise suggesting we do that, but noting that it is a separate, more complex, problem than I'm looking to solve in the short term. For now I'll settle for moving the classloading responsibility into the bean.

> For instance, if there's no setter defined then it has to be one that can only be defined at start-up and not programmatically

which misses the point of DI. There has to be a setter or the framework (Spring/JBossMC) can't set a value in the first place. The startup IS programmatic. Even our standalone propertyFile->Bean loading works by reflection on the setters now.

[jhalliday]

Anyone going to complain if I break things? The current naming convention is:

  String getFoo(); // returns a class name

but keeping that means introducing e.g.

  Foo getFooInstance();

whereas I think we would be better off in the long run with

  String getFooName() || String getFooClassname() || String getFooClassName()

  Foo getFoo();

In the short term however that's going to break every existing config file. Again. I can probably fix the jbossts-properties.xml ones by adding a kludge to the properties loading code, but that still leaves a compatibility problem with existing -beans.xml files.

[marklittle]

"which misses the point of DI. There has to be a setter or the framework (Spring/JBossMC) can't set a value in the first place. The startup IS programmatic. Even our standalone propertyFile->Bean loading works by reflection on the setters now."

There are some properties that simply cannot be changed once the system is running. So if we must have setters they should do nothing or print a warning and do nothing.

[jhalliday]

> There are some properties that simply cannot be changed once the system is running. So if we must have setters they should do nothing or print a warning and do nothing.

Still not getting it are you. Initially they don't have a value. If the setter does nothing they will never have a value. Thus it's not possible to even start the system properly without working setters on the EnvironmentBeans.  What you really mean is that once the component that uses that value has read the bean and initialized itself accordingly, it should never re-read the bean. That's how it works now, so no problem there. The only snag is that the setter can still be called after startup but will have no appreciable effect, which is a bit confusing for the user if they have not bothered to read the docs that explain that's the expected behavior.

[marklittle]

No, I think I do get it ;-) which is why I said that if the setters are called on these "set once for all time, or at least only once per execution of the JVM" attributes then the most we should do for users is print a warning.

[jhalliday]

Fine. In the short term that won't work because the boot sequence includes initializing the bean from the properties file and then from the '3rd party' bean injection framework (JBossMC), so we can't base the locking on number of setter invocations. We would need an explicit 'getAndLock' rather than 'get' to allow the point of use to disallow further changes. Which really is a special case of the aforementioned event mechanism and winds up looking a lot like javabeans property change listener/veto.

[jhalliday]

ok, the basic utility code is in and a single property has been moved to the new model as an example. Now would be a good time to scream if you don't like the way things are shaping up. http://fisheye.jboss.org/changelog/JBossTS?cs=32338

As regards moving static to non-static fields at point of use, taking the above modified CheckedActionFactory as an example, it would be easy to lookup the factory from the bean in each BasicAction ctor, or even lazily in checkChildren. The effect of that change would be that a new factory value set on the bean would take effect from the next transaction begun (or ended) instead of never.

[jhalliday]

Well it has been a while so it's probably time to dust off the franchise and pump out another sequel...

In this exciting installment we'll consider bean instances and problems arising from having more than one of them.

Right now we have a set of config beans that are effectively singletons. The config file reflects this: it's a flat namespace of <BeanType>.<BeanProperty>=value settings. There are two problems with this. First, some beans include properties that are relevant only to a small subset of configurations, in some case mutually exclusive in their use. For example, the ObjectStoreEnvironmentBean contains settings for at least three different ObjectStore implementations, of which typically only one will be in use at a time. Add a new ObjectStore impl and you have to add any impl specific config to the bean. argg. This pushes changes to internal impl details into an otherwise stable pseudo-public api. more argg.  Secondly, you can't have more than one instance of e.g. the same store impl, because there is no way to give each instance its own config unless you start doing horrible hacks like having multiple sets of properties on the env bean: cacheStoreSizeA, cacheStoreSizeB, etc. Cue additional arggggg.

The very basic bean initialization we have right now essentially deals with beans in isolation from one another - no injection support at all. Linking is all done by static fields in the code. Also no identity support, since in the absence of injection you have no reason to say 'give me instance ${x}'. There can be only one.

Unfortunately we've reached the point where this is no longer adequate. So, it's time to either bite the bullet and introduce a dependency on a DI framework, or extend our existing config mechanism with just enough wiring functionality to deal with the immediate problem.

At minimum we need to be able to:

- Store config to allow instantiation of multiple instances of the same class. We could do that be extending the existing property format of <BeanType>.<BeanProperty>=value to be <BeanType>[.<InstanceName>].BeanProperty=value, which has the virtue of retaining compatibility with existing config files and not requiring our own xml schema as well as continuing to allow overriding of value by System properties.

- Allow internals to specify which instance they want to 'inject'. The StoreManager for example needs to instantiate three ObjectStore instances, which may be of assorted types. Something like StoreManager.actionStore=<BeanType>[.<beanName>] may work. More generally <BeanType>.<staticField>=<BeanType>.<instanceName> would provide for minimalist wiring and cover most of our immediate use cases.

I'm wary of spending too much time reinventing the wheel - there are already a significant number of more elegant and capable wiring frameworks available. OTOH most have a footprint considerable larger than JBossTS, even counting all its existing required dependencies. Furthermore they don't always play nice together, so using one may limit our ability to embed into environments controlled by another.