POJOization of config elements

[brian.stansberry]

Branching off from http://www.jboss.com/index.html?module=bb&op=viewtopic&t=92700 as that thread's a bit wide ranging :)

It's nice if individual BuddyLocator, CacheLoader and EvictionPolicy impls can provide POJO config classes; i.e. classes that expose bean properties, rather than config solely via a Properties object. This should be optional; i.e. an impl must be able to configure itself via Properties, but if it has an impl-specific config class that the MC can use, that's good too.

This leads to some API changes. I'll use BuddyLocator as an example.

First, we add a new BuddyLocatorConfig class.

public static class BuddyLocatorConfig {

 public String getBuddyLocatorClass() {...}
 public void setBuddyLocatorClass(String className) {...}

 public Properties getBuddyLocatorProperties() {...}
 public void setBuddyLocatorProperties(Properties properties) {...}

}

Specific BuddyLocator impls can have their own type-specific subclass of this with their own java bean properties, but they must be able to configure themselves via the base superclass w/ its Properties as well.

So, BuddyLocator interface becomes:

public interface BuddyLocator
{
 public BuddyLocatorConfig getConfig();
 public void init(BuddyLocatorConfig config);
 public List locateBuddies(Map buddyPoolMap, List currentMembership, IpAddress dataOwner);
}

Basically init() takes a BuddyLocatorConfig instead of Properties; plus I added a getter for the config.

Similar concept would apply to CacheLoader and EvictionPolicy. For CacheLoader, the equivalent to BuddyLocatorConfig already exists with IndividualCacheLoaderConfig; it's just not directly injected into a CacheLoader. So for the CacheLoader interface setConfig() would take IndividualCacheLoaderConfig instead of Properties, plus I'd add a getter for the config as well.

Haven't looked int detail at EvictionPolicy, but expect the same concept to apply there.

Comments?

[manik]

How would implementation-specific values be set? Again, using your BuddyLocator example, I presume this would be using BuddyLocatorConfig.getBuddyLocatorProperties()? We're back to dealing with properties objects; how does this help the MC, except for the case of *known* setters/getters such as getBuddyLocatorClass()?

As an alternative approach (haven't spent *too* many cycles on this):

1) Use a BuddyLocatorConfig that just contains *known* setters/getters, such as BuddyLocatorClass.

2) If people wish to implement their own BuddyLocator, they would also provide their own sublass to BuddyLocatorConfig, adding their own custom setters/getters.

3) The MC would still be able to inject this BLC subclass into the custom BL impl, since the interface methods comply. The BL impl will be responsible for casting the BLC to the BLC subclass and extracting impl-specific configs.

Thoughts?

[brian.stansberry]

I think we're about 90% on the same page :-)

How would implementation-specific values be set? Again, using your BuddyLocator example, I presume this would be using BuddyLocatorConfig.getBuddyLocatorProperties()? We're back to dealing with properties objects; how does this help the MC, except for the case of *known* setters/getters such as getBuddyLocatorClass()?

The get/setBuddyLocatorProperties() method isn't *really* meant for use by the MC. It's there to support the old style config, where we parse the existing format. AFAIK, we still need to support that.

1) Use a BuddyLocatorConfig that just contains *known* setters/getters, such as BuddyLocatorClass.

Yep. But this base class is the one that gets instantiated if we use the old style config. So, the get/setProperties method needs to be there as well to have some way to pass in the config properties if the MC isn't used.

2) If people wish to implement their own BuddyLocator, they would also provide their own subclass to BuddyLocatorConfig, adding their own custom setters/getters.

Yes, that's what I intend. However, if your BuddyLocator impl is ever going to be used w/ an old-style config file, your BuddyLocator impl needs to also be able to configure itself from a basic BuddyLocatorConfig instance's getProperties() method.

A custom BL impl only used with the MC may not ever concern itself with the Properties; but the standard ones we ship w/ JBC need to be able to configure themselves via Properties.

3) The MC would still be able to inject this BLC subclass into the custom BL impl, since the interface methods comply. The BL impl will be responsible for casting the BLC to the BLC subclass and extracting impl-specific configs.

If the BL impl needs to support the old style config method, it does this when the BLC is passed in:

a) Check if the BLC is of its custom type. If yes, cast it and use it directly to configure yourself.
b) If not, it's a base BLC type created from the old style config. Configure yourself from its properties. (Typically I'd do that by passing the base BLC instance to a c'tor of my custom BLC subclass. The subclass then knows how to build itself from the Properties. The BL then uses the custom BLC instance internally. This approach is an implementation detail though.)

I'm not thinking in terms of the MC injecting a BLC into a BL. The MC does this:

1) builds a complex, composite Configuration object. A BLC is just a sub-component of that. MC creates a BLC, injects that into a BuddyReplicationConfig, which is then injected into the top level Configuration.

2) MC builds the Cache by passing the Configuration into the factory. Then the internal JBC code builds the cache based on the Configuration; BL gets instantiated and passed its config as part of that. I think the rules for building a Cache are too complex to try to have the MC do it directly; the factory provides a good encapsulation.

[manik]

Ok, cool. This sounds good.

A custom BL impl only used with the MC may not ever concern itself with the Properties; but the standard ones we ship w/ JBC need to be able to configure themselves via Properties.

Why doesn't the standard BL impl we ship with (there is only one) also come with a BLC subclass, could be a good example since Properties are ugly and we don't really want to promote that mechanism anyway?

Why do we need to support Properties anyway, for 2.0.0 we don't need to be backward-compat ...

[brian.stansberry]

Why doesn't the standard BL impl we ship with (there is only one) also come with a BLC subclass, could be a good example since Properties are ugly and we don't really want to promote that mechanism anyway?

I've written one. Also, the same pattern applies to CacheLoaders (substitute CacheLoaderConfig for BuddyReplicationConfig and IndividualCacheLoaderConfig for BuddyLocatorConfig.) I've got specific IndividualCacheLoaderConfig subclasses created for all the standard CacheLoader impls as well.

Why do we need to support Properties anyway, for 2.0.0 we don't need to be backward-compat ...

We still want to support creation of a config using the old 1.x format and XmlConfigurationParser, don't we? Otherwise we require use of the MC and a -beans.xml file, even outside the AS. The XmlConfigurationParser approach can't handle arbitrary sets of XML elements (at least not without a lot of work), hence the use of Properties as a workaround.

Even if we decide to require use of the MC in 2.0.0.GA, leaving in Properties support for now makes it easier to transition. We can strip it out later (e.g. after alpha).


FYI, I've been proceeding along the lines discussed on this thread trying to see if there are problems. Here's where it stands:

1) Custom config classes created for all NextMemberBuddyLocator, and all the CacheLoader impls, and impls designed to use them.
2) AFAICT, there are no testsuite regressions using this code (i.e. all the existing tests that use properties for configuration pass. The various config classes all know how to convert from the Properties format to POJO properties, so the existing testsuite is excercising this stuff fairly well.

TODO:

1) Continue discussion.

2) If it sounds good, continue along same lines with eviction policies. This might be easier, as it looks like the eviction policy config is already somewhat done along these lines; e.g. a lot of specialized config classes exist.

2) Set up -beans.xml files for use in the AS to test building the standard cache configs used in the AS. That's provides a basic test of the MC approach.

3) Round out the JBC test suite with more formal tests of the MC approach. I'd say that can be done post-alpha.

How's that sound?

[manik]

We still want to support creation of a config using the old 1.x format and XmlConfigurationParser, don't we? Otherwise we require use of the MC and a -beans.xml file, even outside the AS. The XmlConfigurationParser approach can't handle arbitrary sets of XML elements (at least not without a lot of work), hence the use of Properties as a workaround.

Even if we decide to require use of the MC in 2.0.0.GA, leaving in Properties support for now makes it easier to transition. We can strip it out later (e.g. after alpha).

Good point, +1.

The rest sounds good, I'm for it.

[brian.stansberry]

OK, hopefully I can get the eviction stuff done this evening in the airport. I'll try to do the MC test in the AS over the weekend, but not sure if will be done your Mon AM (wife/kid get a vote :).

If you haven't can you move the InvocationContext() stuff to Cache? That will make testing easier, as not having it there breaks some of the current session repl code. If you don't want to I can work around that, but a bit more work.

[manik]

If you haven't can you move the InvocationContext() stuff to Cache? That will make testing easier, as not having it there breaks some of the current session repl code. If you don't want to I can work around that, but a bit more work.

done already. :)

[brian.stansberry]

As mentioned on the dev list, a lot has been checked in on this. There are some bits of ugliness. Manik, you pointed out the problem with the use of inner classes; I should be able to fix that today.

Other ugliness:

1) MC doesn't like overloading the setter on a property, which is an issue for the properties that are enum types. E.g.:

public CacheMode getCacheMode()
public void setCacheMode(CacheMode mode)
public void setCacheMode(String mode)

doesn't work; the MC wants to pass String "REPL_ASYNC" to the overloaded method that takes CacheMode, and thus blows up.

To fix, I had to do this for CacheMode, IsolationLevel and NodeLockingScheme:

public CacheMode getCacheMode()
public void setCacheMode(CacheMode mode)
public void setCacheMode(String mode) // maybe redundant??
public String getCacheModeString()
public void setCacheModeString(String mode)

I don't know if it's practical to add a PropertyEditor to MC to handle enum conversions; for now the above is ugly but works.


2) Names of properties in Configuration. Configuration can take 2 different types for each of the 3 complex sub-configurations (BR, cache loading, eviction). These are an Element, for legacy support, and a pojo for configuration via the MC. So, 2 properties for each sub-configuration. Problem is naming these properties; the Element properties already have names, but the naming conventions are inconsistent. Changing the Element properties will break existing config files, so I worked around that when naming the pojo properties. Ugly, completely inconsistent result is:

// BR
public Element getBuddyReplicationConfig()
public void setBuddyReplicationConfig(Element config)

public BuddyReplicationConfig getBuddyReplicationConfiguration()
public void setBuddyReplicationConfiguration(BuddyReplicationConfig config)

// Cache Loader
public Element getCacheLoaderConfiguration()
public void setCacheLoaderConfiguration(Element config)

public CacheLoaderConfig getCacheLoaderConfig()
public void setCacheLoaderConfig(CacheLoaderConfig config)

// Eviction
public Element getEvictionPolicyConfig()
public void setEvictionPolicyConfig(Element config)

public EvictionConfig getEvictionConfig()
public void setEvictionConfig(EvictionConfig config)

One temptation is to break compatibility with existing config files for BR by swapping the property names. There aren't likely to be many existing configs that have BR. E.g.

public Element getBuddyReplicationConfiguration()
public void setBuddyReplicationConfiguration(Element config)

public BuddyReplicationConfig getBuddyReplicationConfig()
public void setBuddyReplicationConfig(BuddyReplicationConfig config)

3) Eviction configuration naming. The Pojo configuration looks like this:

class EvictionConfig -- top level object. Configure your default eviction policy here, plus the interval for the eviction thread. Also takes a list of...

class EvictionRegionConfig -- one per region. Configure the Fqn here, plus the class name of the EvictionPolicy (if you want to override the overall default). Also takes an impl of....

interface EvictionConfiguration. the type-specific config for the EvictionPolicy.

Couple problems here. First "EvictionConfiguration" would be better named "EvictionPolicyConfig", since its a config for a specific policy. But, that interface already exists in 1.4, so changing that name will break any existing custom impls. Don't know if we care about that. Second, there's already an unused interface named EvictionPolicyConfig, along with a DefaultEvictionPolicyConfig impl. That code is unused; looks like it was meant more as a top-level config object. We should drop that code after ensuring that any design ideas from it get picked up in the new stuff.


Any thoughts/suggestions are most welcome.

[starksm64]

As mentioned on the dev list, a lot has been checked in on this. There are some bits of ugliness. Manik, you pointed out the problem with the use of inner classes; I should be able to fix that today.

Other ugliness:

1) MC doesn't like overloading the setter on a property, which is an issue for the properties that are enum types. E.g.:

public CacheMode getCacheMode()
public void setCacheMode(CacheMode mode)
public void setCacheMode(String mode)

doesn't work; the MC wants to pass String "REPL_ASYNC" to the overloaded method that takes CacheMode, and thus blows up.

To fix, I had to do this for CacheMode, IsolationLevel and NodeLockingScheme:

public CacheMode getCacheMode()
public void setCacheMode(CacheMode mode)
public void setCacheMode(String mode) // maybe redundant??
public String getCacheModeString()
public void setCacheModeString(String mode)

I don't know if it's practical to add a PropertyEditor to MC to handle enum conversions; for now the above is ugly but works.

It should not require a PropertyEditor as there already is the Enum.valueOf(Class enumType, String name) method. Its just a question of xml configuration parsing layer recognizing the overloaded method and using valueOf on the string accessor. I'll create a jira issue for this.

2) Names of properties in Configuration. Configuration can take 2 different types for each of the 3 complex sub-configurations (BR, cache loading, eviction). These are an Element, for legacy support, and a pojo for configuration via the MC. So, 2 properties for each sub-configuration. Problem is naming these properties; the Element properties already have names, but the naming conventions are inconsistent. Changing the Element properties will break existing config files, so I worked around that when naming the pojo properties. Ugly, completely inconsistent result is:

// BR
public Element getBuddyReplicationConfig()
public void setBuddyReplicationConfig(Element config)

public BuddyReplicationConfig getBuddyReplicationConfiguration()
public void setBuddyReplicationConfiguration(BuddyReplicationConfig config)

// Cache Loader
public Element getCacheLoaderConfiguration()
public void setCacheLoaderConfiguration(Element config)

public CacheLoaderConfig getCacheLoaderConfig()
public void setCacheLoaderConfig(CacheLoaderConfig config)

// Eviction
public Element getEvictionPolicyConfig()
public void setEvictionPolicyConfig(Element config)

public EvictionConfig getEvictionConfig()
public void setEvictionConfig(EvictionConfig config)

One temptation is to break compatibility with existing config files for BR by swapping the property names. There aren't likely to be many existing configs that have BR. E.g.

public Element getBuddyReplicationConfiguration()
public void setBuddyReplicationConfiguration(Element config)

public BuddyReplicationConfig getBuddyReplicationConfig()
public void setBuddyReplicationConfig(BuddyReplicationConfig config)

We should not be exposing any dom objects via the configuration objects. That should only be done via legacy configuration wrapper classes that turn around and write to the pure object based configuration instance.

[manik]

One temptation is to break compatibility with existing config files for BR by swapping the property names. There aren't likely to be many existing configs that have BR. E.g.

Not true. This is for JBC2.0.0/AS5. BR is available in AS 4.0.5, and I suspect people will start using it.

public Element getBuddyReplicationConfiguration()
public void setBuddyReplicationConfiguration(Element config)

public BuddyReplicationConfig getBuddyReplicationConfig()
public void setBuddyReplicationConfig(BuddyReplicationConfig config)

Ugh! :-) I'd steer well clear of such ambiguities, if I were you! If we can use a PropertyEditor where needed, I'd prefer this approach.

[manik]

To add perspective, the above comment was in response to point #2 that you mentioned.

[manik]

Re: #1, as Scott suggested, you shouldn't need a PropertyEditor for Enums with the MC.

And Re: #3:

3) Eviction configuration naming. The Pojo configuration looks like this:

class EvictionConfig -- top level object. Configure your default eviction policy here, plus the interval for the eviction thread. Also takes a list of...

class EvictionRegionConfig -- one per region. Configure the Fqn here, plus the class name of the EvictionPolicy (if you want to override the overall default). Also takes an impl of....

interface EvictionConfiguration. the type-specific config for the EvictionPolicy.

Couple problems here. First "EvictionConfiguration" would be better named "EvictionPolicyConfig", since its a config for a specific policy. But, that interface already exists in 1.4, so changing that name will break any existing custom impls. Don't know if we care about that. Second, there's already an unused interface named EvictionPolicyConfig, along with a DefaultEvictionPolicyConfig impl. That code is unused; looks like it was meant more as a top-level config object. We should drop that code after ensuring that any design ideas from it get picked up in the new stuff.

Yes, this stuff needs a bit of cleaning up. I suggest:

1) Leave EvictionConfiguration as it is.
2) EvictionPolicyConfig extends EvictionConfiguration, and internally we use EvictionPolicyConfig. We also document EvictionPolicyConfig and suggest that people use this, giving us a migration path.
3) Clean up on the unused code around here.
4) Remove the now redundant EvictionPolicyClass attribute - as this is now set as a part of a Region.

What do you think? I don't mind removing EvictionConfiguration altogether and breaking 1.4.0 compat as we're doing this in other areas anyway, but we aren't changing that much in the eviction policies so I'm not sure about this.

[brian.stansberry]

Ugh! :-) I'd steer well clear of such ambiguities, if I were you! If we can use a PropertyEditor where needed, I'd prefer this approach.

Following Scott's suggestion, all such ambiguities will be gone. The Configuration object will just expose:

public BuddyReplicationConfig getBuddyReplicationConfig()
public void setBuddyReplicationConfig(BuddyReplicationConfig config)
public CacheLoaderConfig getCacheLoaderConfig()
public void setCacheLoaderConfig(CacheLoaderConfig config)
public EvictionConfig getEvictionConfig()
public void setEvictionConfig(EvictionConfig config)

The task of interpreting the legacy element names in XML will be handled by XmlConfigurationParser.