JBAS-5647 HAPartition Sync Call with RspFilter

[galder.zamarreno]

Hi,

re: http://jira.jboss.com/jira/browse/JBAS-5647

Taking in account that this JIRA requires an API change, I'd like to get it done asap. Creating the response filter for HA-JNDI and making HA-JNDI use this new API can come a bit later. At the JGroups level, the API that we need to call to pass the RspFilter is:

public RspList callRemoteMethods(Vector dests, MethodCall method_call, int mode, long timeout,
 boolean use_anycasting, boolean oob, RspFilter filter)

At the HAPartition level, we can either change callMethodOnCluster, or overload it to add, at least, RspFilter. Overloading is probably safer from a backwards compatibility perspective. This is for AS 5.0.0.GA, so I guess backwards compatibility is not a top priority. I'm easy with either.

The second thing to decide is what to do about the extra parameters that callRemoteMethods takes which are:

boolean use_anycasting, boolean oob

Do we wanna expose them to HAPartition clients? In the case of anycast, I think this would only make sense if the destination members where exposed in the HAPartition API, otherwise, to the HAPartition client, it's as if it was calling the method to all members in the cluster. Anycast only makes sense when you're calling on a subset of the cluster. In the case of oob, messages sent like this are used for non-application usages such as FD heartbeats (see http://jira.jboss.com/jira/browse/JGRP-205), so I'd leave it out again.

Thoughts?

[brian.stansberry]

No to anycasting and oob. No feature without a clearly defined use case, particularly not at this stage of the game. :-)

It needs to be an overloaded method.

Last but not least, we can't leak the RspFilter interface through the HAPartition API. HAPartition is meant to be an abstraction that hides the use of JGroups. Originally, when JGroups and AS clustering was new, hiding it to allow using something else underneath. That's unlikely to happen but I don't want to break the abstraction. More directly relevant, the ha-server-api project has no dependency on JGroups, and thus consumers of it don't either. I want to keep it that way.

I think what we need is an org.jboss.ha.framework.interfaces.ResponseFilter interface (spelled out, not RspFilter) in ha-server-api. Then in the AS cluster module org.jboss.ha.framework.server an adapter class that implements RspFilter and delegates to a constructor-injected ResponseFilter.

ResponseFilter can't expose Address either; should be ClusterNode. The adapter class needs to do the translation. :(

[brian.stansberry]

As an optimization, ClusterPartition can check if a ResponseFilter passed to callRemoteMethods also implements RspFilter. If so, just pass it through, don't wrap it. The filter used by HA-JNDI could be such a filter.

[galder.zamarreno]

No to anycasting and oob. No feature without a clearly defined use case, particularly not at this stage of the game. :-)

It needs to be an overloaded method.

Fine with both

Last but not least, we can't leak the RspFilter interface through the HAPartition API. HAPartition is meant to be an abstraction that hides the use of JGroups. Originally, when JGroups and AS clustering was new, hiding it to allow using something else underneath. That's unlikely to happen but I don't want to break the abstraction. More directly relevant, the ha-server-api project has no dependency on JGroups, and thus consumers of it don't either. I want to keep it that way.

I think what we need is an org.jboss.ha.framework.interfaces.ResponseFilter interface (spelled out, not RspFilter) in ha-server-api. Then in the AS cluster module org.jboss.ha.framework.server an adapter class that implements RspFilter and delegates to a constructor-injected ResponseFilter.

ResponseFilter can't expose Address either; should be ClusterNode. The adapter class needs to do the translation. :(

Thanks for the very thorough answer. Makes sense.

[galder.zamarreno]

More directly relevant, the ha-server-api project has no dependency on JGroups, and thus consumers of it don't either...

Hmm, actually ha-server-api/pom.xml shows a dependency on JGroups. I suppose this is the result of ha-server-api depending on JBC, which would indirectly bring JGroups. I understand your point though.

[galder.zamarreno]

As an optimization, ClusterPartition can check if a ResponseFilter passed to callRemoteMethods also implements RspFilter. If so, just pass it through, don't wrap it. The filter used by HA-JNDI could be such a filter.

Hmmm, I'm a bit confused here. I clearly understand the performance optimisation here but by doing so, aren't you just going against the abstraction of the JGroups layer? Ok, maybe not entirely against it because clients do need to adhere to passing a ResponseFilter, and whether they pass a RspFilter as well it's up to them but you're effectively promoting implementations to directly tie themselves to RspFilter.

[galder.zamarreno]

I'm trying to play a bit devil's advocate here. Optimisations are important and I suppose sometimes there're some trade-offs that you have to make to achieve them.

[brian.stansberry]

Yeah, it's kind of hacky, probably a bad idea. Don't do it if the overhead of translating from Address to ClusterNode in the adapter class is small.

HA-JNDI is in the cluster module, which has a JGroups dependency. So a class that can satisfy both ResponseFilter and RspFilter doesn't introduce any new dependency.

The hacky thing is ClusterPartition looking at an object of type ResponseFilter, seeing it implements RspFilter, and treating it as a RspFilter even though it was not presented that way.

Good catch on the pom dependency. I need to look why that's there (rather than just being transitive via the optional JBC dependency.) Probably because JBC 2.1.0.GA didn't depend on the JBC release I wanted in the AS, but I don't see why I'd want JGroups in the ha-server-api pom because of that.

[galder.zamarreno]

Yeah, it's kind of hacky, probably a bad idea. Don't do it if the overhead of translating from Address to ClusterNode in the adapter class is small.

The overhead looks small, primarily involving some string manipulation. I see that DNS lookups are now avoided thanks to http://jira.jboss.com/jira/browse/JBAS-3794. Clever.

I'll make sure I make a note in the code of this potential optimisation in case we wanna implement it at a later stage.

[brian.stansberry]

String manipulation? I was thinking in terms of some kind of map lookup.

Ah, you're creating new ClusterNodeImpls.

If ClusteredNodeImpl.jgId were made volatile and lazy initialized in getJGName() the constructor would be pretty fast. The getJGName() method isn't even in the ClusterNode interface; I just left the method in the impl when I created the interface just in case someone was using it.

[brian.stansberry]

I see that DNS lookups are now avoided thanks to http://jira.jboss.com/jira/browse/JBAS-3794. Clever.

Clever but poorly done. :-) ClusterNodeImpl.getFastHostName was incorrectly implemented; I just checked in the fix.

Also. 1.1.0.CR2 of the ha-xxx libs has been released.

[galder.zamarreno]

String manipulation? I was thinking in terms of some kind of map lookup.

Ah, you're creating new ClusterNodeImpls.

That looked the simplest option but I must admit that I did think on whether Address to ClusterNode mappings are/should be stored in a map to avoid creation of ClusterNodeImpls for each RPC call. I'll double check to see whether there's something in place for this.

[brian.stansberry]

Good that you thought about it. But don't get me wrong, I think creating new ClusterNodeImpls is fine if you make jgId lazy initialized. Simple is good. :) And the whole idea of this is to return (and stop converting from Address to ClusterNode) as soon as 1 good response comes in.

[brian.stansberry]

One problem I see in this is handling of ClusterPartition.NoHandlerForRPC, which is what ClusterPartition.RPCHandler returns if the requested service name isn't registered on that node. ResponseFilter impls will need to know how to deal with that, which means making the class a public top level class.

For example, a ResponseFilter for HA-JNDI calls shouldn't accept a NoHandlerForRPC and then return true from needMoreResponses().