Related discussion: in HA-JNDI this attribute is named "ClusterPartition". Elsewhere it's named "Partition". Since we're changing things, now's the time to clean things up and make them consistent.

Three choices:

1) Change HA-JNDI and call it "Partition".
2) Change others to "ClusterPartition". I reject this because ClusterPartition is the name of a particular impl of HAPartition.
3) Change everything to "HAPartition".

Any thoughts on this Jerry?

Hmm. Hoisted (sic??) by my own petard.

Seems it's pretty consistently "ClusterPartition". HAServiceMBean as well; that's the class end users are likely to use. I named the attribute "ClusterPartition" when I added it for dependency injection of the ClusterPartitionMBean.

OK, then, "Partition" is out. Only reason it was on the list is I thought I'd used it.

I'm inclined to say make it "HAPartition" since the change of the attribute type breaks existing clients anyway.

(I'm not that concerned about client breakage due to API change; AS 5 is going to be a big change in general, and the "ClusterPartition" attribute is only about 1 year old, and was added for internal purposes, not to support some user feature.)

How about this (comments/arguments appreciated):

1) Make it "HAPartition".
2) The one place where the old "ClusterPartition" attribute was likely to be used by end users was HAServiceMBean. People could use it in -service.xml to inject the ClusterPartition. So there we *add* the "HAPartition" attribute, deprecate "setClusterPartition" and remove getClusterPartition. A call to setClusterPartition just calls setHAPartition(clusterPartition.getHAPartition()).

As for getPartitionName(), I just blindly deprecated both the getter and setter. I don't mind un-deprecating the getter.

"JerryGauth" wrote:

I'm not familiar with the use of HAServiceMBeanSupport/HAServiceMBean. The javadoc and class names suggest that this class is provided to allow clients to extend the implementation and integrate with their own clustered mbeans. If we modify the mbean/implementation so that clients access the cluster's interface rather than its mbean, does that affect clients who might want access to the mbean itself?

Let's throw an exception and strip out any code that tries to do a lookup. If the partition isn't available, that means Brian screwed up when he wrote and tested the config file. :)

In general, I want to go with a straight IOC approach -- services assume they have dependencies injected, and don't try looking them up themselves. If the necessary dependencies aren't available, throw an IllegalStateException (let's avoid any NPE).

AIUI, this business of looking stuff up predates the days when the JMX Microkernel could do dependency injection via "depends optional-attribute name". I would have torn it out when I added the "ClusterPartition" attribute, but that was too big a change for a 4.0 point release.

We may need to be a bit more cautious/think about it a wee bit more in the case of things like HAServiceMBeanSupport, where it's expected users will subclass the class. But for internal services like HASessionState, be ruthless. :)

I'm going to think out loud here, as this gets into an area where things aren't too clean. Apologies in advance for what I'm sure will ramble on.... I tend to think things through in writing.

ClusterPartition is really playing two roles right now:

1) It's an impl of HAPartition that uses JGroups as an implementation detail. This is it's primary function.

2) There are services that use HAPartition for group RPCs and a JBC instance for maintaining shared state. These services need a guarantee that the HAPartition and the JBC instance are using a consistent underlying group comm layer (i.e. the group members for the HAPartition and for the cache are the same). ClusterPartition is acting as a guarantor. If you call getClusteredCache() and getHAPartition(), the two services are guaranteed to be using the same underlying JChannel.

ClusterPartitionMBean should really just be a management interface for ClusterPartition -- i.e. something used by management tools like JBoss Operations Network or jmx-console. It shouldn't be used by clients like HA-JNDI. It also really shouldn't expose methods not needed by the mgmt tools. In this respect, ClusterPartitionMBean is pretty reasonable as a mgmt interface. The suspect methods are getClusteredCache(), getDistributedStateService() and getHAPartition() -- it's not clear what a mgmt tool would do with those methods. getMultiplexer() could be on the suspect list too, although I could see how a tool might want to navigate from ClusterPartitionMBean to the ChannelFactory.

So, HA-JNDI shouldn't be using the mgmt tool interface to call getHAPartition() and getClusteredCache(). Which gets us to your question: what should it be using:

1) Don't want to add getClusteredCache() to HAPartition. We could (and should) have a separate discussion about what HAPartition is/should be. But in general its an abstraction of a group RPC layer, somewhat intended to hide JGroups. It then has two services bolted on via getters (DRM and DistributedState). Those really don't have anything to do with the RPC functionality other than since the interface clubs them together a client can assume the DRM/DS and RPCs are using the same underlying group comm channel. Hmm -- sounds familiar.

Adding another getter to expose JBC further confuses things by pushing HAPartition away from its core function as a group RPC abstraction. It also goes against the idea that HAPartition hides JGroups. JBC is very much tied to JGroups.

2) Inject HAPartition, cast to ClusterPartition and make the getClusteredCache() call. Nah.

3) For DetachedHANamingService, injection type is ClusterPartition instead of HAPartition. Remove the attribute from DetachedHANamingServiceMBean -- tools don't need it; the microcontainer does, and a public method on the class itself is all the MC needs. The fact that ClusterPartition is injected into the service impl is an implementation detail. This solves the problem of HANamingService making calls against ClusterPartitionMBean.

4) Make a new interface that encapsulates the "guarantor of consistent HAPartition and JBC" function. (I have no good name for it.) Exposes getHAPartition() and getClusteredCache(). ClusterPartition can implement this interface. For DetachedHANamingService, injection type is this interface. This use of an interface leaves us free to refactor someday and break ClusterPartition's two roles into separate classes.


I'm inclined toward #4. #3 seems OK for the immediate task but if we do it we should add a subtask to JBAS-4276 to look at it again.

This bumps into the "who guarantees the HAPartition and the Cache are using the same channel" problem. Independently injecting them puts the burden on the person writing the config file; i.e. there is no software enforcement.

I've been tempted several times to go that way. But my gut always tells me it's a mistake.

Sure, but why not make it explicit and just inject the "channel-resolver"? That's basically what my #4 is.