[brian.stansberry]

Not agressive it all. :-) An important subtext here is deciding where this falls on priority lists; knowing whether or not the community actually cares about the issue is important in deciding that.

[galder.zamarreno]

Would #1 be that bad? You create ExtendedHASingletonElectionPolicy with the missing methods from the AS 5 API. If the injected policy implements Extended, you invoke on it the way AS 5 does. Otherwise it gets used as it is now. In ExtendedHASingletonElectionPolicy javadoc, any HASingletonElectionPolicy methods that don't get called anymore should have comments noting that fact.

Don't get me wrong; I can see how it would get messy. #3 is OK.

Hmm, looking at #1 again, I suppose it's the same thing that was done for HAMembershipExtendedListener. I'll go with #1.

I know you did a lot of refactoring to tighten things up in trunk, i.e. with HASingletonElectionPolicyBase; that doesn't need to happen in 4.x, and *shouldn't* happen if it might break some custom subclass. HASingletonElectionPolicySimple just needs to work correctly.

Bingo! The aim for 4.2.x is just fixing it :)

[galder.zamarreno]

I'm back :)

I've got this pretty much nailed down for 4.2.x. Here's a summary of what I've done:

- created org.jboss.ha.singleton.ExtendedElectionPolicy (dropped verbose HASingleton from class name) which contains setSingletonName()/getSingletonName() methods that would allow a extended election policy to query DRM. The benefit of this is that it allows original isElectedMaster() to still be called as it is, but the difference is in the impl (whether extended or not), or more specifically, in the pickSingleton() impl.

- created paralel ExtendedElectionPolicyBase, ExtendedElectionPolicyMBean, ExtendedElectionPolicySimple and ExtendedElectionPolicySimpleMBean to avoid touching any of the existing election policy classes.

- HASingletonSupport overrides setupPartition() to be able to set HAPartition and singleton name before first election vote.

- setting singleton name is done this way to be able to cope with both extended and non extended interface:

try
{
 server.invoke(((MBeanProxyInstance)mElectionPolicyMB).getMBeanProxyObjectName(), "setSingletonName", new Object[] {getServiceHAName()}, new String[] {"java.lang.String"});
}
catch(ReflectionException re)
{
 log.debug("Selected election policy does not support setting singleton name, skipping assignment.");
}

Remember than in 4.x, we set the eviction policy via proxy:

<depends optional-attribute-name="ElectionPolicy"
 proxy-type="attribute">jboss.examples:service=HASingletonMBeanExample-ExtendedElectionPolicySimple_1</depends>

That's pretty much it. There's a final question I wanted to post here for discussion:

The current javadoc for APIHASingletonElectionPolicy.pickSingleton()/HASingletonElectionPolicy.pickSingleton(HAPartition) does not mention the possibility of pickSingleton() returning null. In fact, isElectedMaster calls something like: pickSingleton().equals() so it's definitely not ready to accept a null value out of it.

When the last node in the cluster is stopping, the existing pickSingleton() implementation just choses itself, even though it's undeploying the ha singleton in the very last node. Bottom line: it's stopping the last node but ends up starting the ha singleton when it shouldn't really.

The extended implementation I've worked on can detect this and return null out of pickSingleton(), leading to isElectedMaster returning false. In that situation the HASingleton will not be started as singleton if the last node in the cluster is shutting down.

The 2nd is IMO the correct behaivour but requires HASingletonElectionPolicyBase.isElectedMaster() to be able to cope with a null value being returned from pickSingleton(). If I do that, iow if I return null from pickSingleton(), I'm effectively breaking the original API javadoc for pickSingleton() though.

The fact that the HASingleton is started during the stoppage of the last node in the cluster might not be a big deal, but I suppose on what the HASingleton.startSingleton() does...

Might not be very important at the moment, but all this could be relevant at some point depending on the HASingleton implementations out there and the users' expectations in such circumstances.

Thoughts?

[galder.zamarreno]

A 3rd possibility would be to add a 2rd method to org.jboss.ha.singleton.ExtendedElectionPolicy called something like: pickSingletonIfExists() and call that from HASingletonElectionPolicyBase.isElectedMaster() if exists, otherwise call existing pickSingleton.

pickSingletonIfExists() would return null if no cluster nodes were found.

[brian.stansberry]

Go ahead and return null, update the javadoc to reflect the fact it might return null, and have HASingletonElectionPolicyBase handle the null properly.

The javadoc doesn't say it won't return null. And null is the only logical answer if the service isn't deployed anywhere. So adding a lot of workarounds to deal with it doesn't seem worth it.

[galder.zamarreno]

OK, thanks for the input :)

[galder.zamarreno]

I'll be committing JBAS-5170 in a minute. Just a quick note on something I've added to the resolution of this JIRA:

Together with the call to setSingletonName in HASingletonSupport, I'm also calling setHAPartition(). As a result, pickSingleton(HAPartition) and isElectedMaster(HAPartition) are effectively not needed for neither the standard nor the extended election policy implementations.

So, I'm doing two things here:
- these methods delegate to their overload methods that have no parameters, i.e. pickSingleton() and isElectedMaster()
- In HASingletonElectionPolicy, I'm deprecating pickSingleton(HAPartition) and isElectedMaster(HAPartition) adding an explanation in the @deprecated javadoc.

The code is much more readable now :). If someone disagrees with this, I'll go back and change it. For the time being, I'm committing this.