1 2 Previous Next 17 Replies Latest reply on Jul 12, 2007 4:23 PM by Jerry Gauthier

    JBAS-4276 Discussion Thread

    Brian Stansberry Master

      Discussion of http://jira.jboss.com/jira/browse/JBAS-4276.

      Previous discussion taken from JIRA comments:

      "Jerry Gauthier" wrote:
      Brian - I think you're already done this work. I couldn't find any configuration files in cluster\src\etc where we're injecting a partition name instead of HAPartition.


      Yes, but IIRC there are a lot of places where ClusterPartitionMBean is the type of a property. That's JMX-centric and AS 5 should be POJO-centric. So, we need to look at the interfaces and move toward making HAPartition the property type. Also, no setter for a String partition name.


      Brian - for clarification before I proceed with this work.

      Using the HA-JNDI service as an example, you've already modified the configuration file (cluster-beans.xml) to inject HAPartition instead of the ClusterPartition mbean. However the property type of attribute "clusterPartition" in DetachedHANamingService is ClusterPartitionMBean. I need to change the type to org.jboss.ha.framework.interfaces.HAPartition. Is this correct?


      Yes, this is correct. Also don't want to expose setPartitionName().

        • 1. Re: JBAS-4276 Discussion Thread
          Brian Stansberry Master

          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?

          • 2. Re: JBAS-4276 Discussion Thread
            Jerry Gauthier Apprentice

            The classes I've looked at all use the name "ClusterPartition." Examples include DetachedHANamingService, FarmMemberService and JGCacheInvalidationBridge. I've haven't done an exhaustive seacrh but clearly we would need to modify more than DetachedHANamingService (e.g., HA-JNDI).

            I'm not too enthusiastic about "Partition" as it's such a generic term. Maybe "HAPartition" would be preferable.

            It seems that this change will break clients regardless of what we do about the attribute/method name(s), since the return type of the getter will change from ClusterPartitionMBean to HAPartition.

            Agreed that we'll remove the setPartitionName() method from the classes that inject cluster references. I'll leave getPartitionName() as a convenience method; I noted that you've deprecated it in at least one implementation that I examined.

            • 3. Re: JBAS-4276 Discussion Thread
              Brian Stansberry Master

              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.

              • 4. Re: JBAS-4276 Discussion Thread
                Jerry Gauthier Apprentice

                Ignoring HAServiceMBeanSupport/HAServiceMBean for the moment, here's what we're now agreeing to implement, pending other objections.

                1) Remove the following methods from relevant classes.

                ClusterPartitionMBean getClusterPartition()
                 void setClusterPartition(ClusterPartitionMBean)
                 void setPartitionName(String)


                2) Add the following methods as replacements.
                HAPartition getHAPartition()
                 void setHAPartition(HAPartition)


                3) Undeprecate the following "convenience" method.
                String getPartitionName()


                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?





                • 5. Re: JBAS-4276 Discussion Thread
                  Brian Stansberry Master

                   

                  "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?


                  In AS 5, the MBean is solely meant to be an interface for the use of management tools; it's not a client interface. So, I have no problem with limiting clients to the HAPartition interface.

                  Not really limiting them anyway. If a particular HAServiceMBeanSupport subclass wants access to the mbean, then it's trivial for them to override setClusterPartition and cache a ref to the mbean.

                  • 6. Re: JBAS-4276 Discussion Thread
                    Jerry Gauthier Apprentice

                    I'll modify HAServiceMBeanSupport/HAServiceMBean as suggested, leaving these changes for last in case there are further comments.

                    • 7. Re: JBAS-4276 Discussion Thread
                      Jerry Gauthier Apprentice

                      A question on the SessionStateService -

                      Currently this service instantiates HASessionStateImpl when the service is created. If the partition hasn't been injected, the service just uses the partition name when constructing HASessionStateImpl. The constructor then uses the partition name to ultimately lookup the partition via JNDI. If the partition name is missing, the constructor uses the default JNDI name for the partition.

                      For other services that inject the partition, we throw an exception in the start() method if the partition is null. In this service, we no longer have a partition name if the service hasn't been injected. However we can still pass a null partition name to HASessionStateImpl which will trigger an attempt to perform the default name lookup.

                      I can leave HASessionStateImpl as is so that it will still attempt to use the default name in a JNDI lookup. Alternatively I can throw an exception in HASessionStateService.create() if the partition hasn't been injected.

                      Any thoughts on what's preferable here?

                      • 8. Re: JBAS-4276 Discussion Thread
                        Brian Stansberry Master

                        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. :)

                        • 9. Re: JBAS-4276 Discussion Thread
                          Jerry Gauthier Apprentice

                          While modifying HA-JNDI, I found that it invokes getClusteredCache() on the partition. This method is available on the ClusterPartitionMBean interface but not on the HAPartition interface so I can either add it to the HAPartition interface or hack it by casting the interface reference to the HAPartition implementation.

                          I'll take a closer look at this tomorrow in case it's not appropriate to add it to the interface.

                          • 10. Re: JBAS-4276 Discussion Thread
                            Brian Stansberry Master

                            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.

                            • 11. Re: JBAS-4276 Discussion Thread
                              Jerry Gauthier Apprentice

                              I haven't fully digested your comments yet but upon review of the existing implementation, it looks like a simpler solution may be viable.

                              What about injecting the cache instance into HA-JNDI, similar to what we now do for DistributedState? This should eliminate any need for HA-JNDI to invoke getClusteredCache().

                              • 12. Re: JBAS-4276 Discussion Thread
                                Brian Stansberry Master

                                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.

                                • 13. Re: JBAS-4276 Discussion Thread
                                  Scott Stark Master

                                  Why do you see it that way? Certainly you should be able to inject them, and if its too error prone write a dependency that enforces the behavior:

                                  <bean name="cache">
                                   <property name="channel"><inject bean="channel-resolver" property="cacheChannel" /></property>
                                  </bean>
                                  <bean name="partition">
                                   <property name="channel"><inject bean="channel-resolver" property="partitionChannel" /></property>
                                  </bean>
                                  <bean name="channel-resolver">
                                  ...
                                  </bean>
                                  



                                  • 14. Re: JBAS-4276 Discussion Thread
                                    Brian Stansberry Master

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

                                    1 2 Previous Next