JBAS-5703 - Make HA-JNDI use wait for first positive respons

[galder.zamarreno]

Re: https://jira.jboss.org/jira/browse/JBAS-5703

I've attached a patch containing fix for this. Few notes:

1. I've tested this with a 3 node cluster (node1,node2 and node3) and two ejbs (ejbA and ejbB where ejbA calls ejbB via HAJNDI). Deployment wise, ejbA is deployed in node1 and ejbB is deployed in node3. I was able to make a call successfully via ejbA in node1 and was able to check via logs that once response from node3 was received, the lookup returned. Now, my question is, how to create a unit test around this? Not easy...

2. I noted this as a todo in the patch: ResponseFilter/RspFilter API: If needMoreResponses() took response and sender as well, class implementations could be Immutable and same instance could be passed to all lookups. As it is right now, they can't and new instances need to be generated for every lookup.

3. Could response filtering done in ClusterPartition.processResponseList() be moved here? Any NoHandlerForRPC would be caught here immediately and could potentially avoid double processing. Thoughts?

[brian.stansberry]

Re: https://jira.jboss.org/jira/browse/JBAS-5703

I've attached a patch containing fix for this. Few notes:

Great; thanks.

1. I've tested this with a 3 node cluster (node1,node2 and node3) and two ejbs (ejbA and ejbB where ejbA calls ejbB via HAJNDI). Deployment wise, ejbA is deployed in node1 and ejbB is deployed in node3. I was able to make a call successfully via ejbA in node1 and was able to check via logs that once response from node3 was received, the lookup returned. Now, my question is, how to create a unit test around this? Not easy...

Testing can be done in layers:

a) Unit test your LookupSucceededFilter. That's really the key thing here.
b) Unit test RspFilterAdapter.
c) A test of ClusterPartition.callMethodOnCluster where ResponseFilter param is passed. One way to do that at least partially is to have a service that calls method public boolean echo(boolean arg), where response is whatever arg was. Call with a filter that only accepts "true". Invoke method twice, passing "true" then "false"; should get a response to first call, none to second call.
d) A full test of the HA-JNDI functionality. That can wait until we make ClusterPartition more of a POJO, something where a couple can be instantiated in a unit test the way JBC can. You can file a JIRA to add such a test.

2. I noted this as a todo in the patch: ResponseFilter/RspFilter API: If needMoreResponses() took response and sender as well, class implementations could be Immutable and same instance could be passed to all lookups. As it is right now, they can't and new instances need to be generated for every lookup.

Having 2 methods lets you filter responses out of the response list while still accepting more responses. To get the same thing out of one method you'd have to change the return type to something beyond simple boolean, perhaps Boolean but better an enum. This would be a JGroups API change.

3. Could response filtering done in ClusterPartition.processResponseList() be moved here? Any NoHandlerForRPC would be caught here immediately and could potentially avoid double processing. Thoughts?

Something to think about; could be done in RspFilterAdapter.

Hmm -- don't think it works well for the ordinary case where you want responses from everyone. To return properly from needMoreResponses() the RspFilterAdapter would have to know how many responses it is expecting and count how many it has gotten. And deal with suspicions/view changes that may occur in the meantime.

[galder.zamarreno]

2. I noted this as a todo in the patch: ResponseFilter/RspFilter API: If needMoreResponses() took response and sender as well, class implementations could be Immutable and same instance could be passed to all lookups. As it is right now, they can't and new instances need to be generated for every lookup.

Having 2 methods lets you filter responses out of the response list while still accepting more responses. To get the same thing out of one method you'd have to change the return type to something beyond simple boolean, perhaps Boolean but better an enum. This would be a JGroups API change.

I'm not sure you understood what I meant. I agree that we need 2 methods but why not have needMoreResponses() also receive the response and the sender? The problem right now is that I know whether I need more responses based on what isAcceptable() receives!! This makes the filter stateful, I need to cache a boolean and return that in needMoreResponses(). IOW, what I'm asking is that both methods, which have different functions, to receive both the response and sender. That would simplify the ResponseFilter implementation making it Immutable and hence you can always pass the same instance to the remote method call.

I know it's an API change but something to keep in mind for JGroups 3.0?

[brian.stansberry]

OK, that makes very good sense. Really Bela's call; best to raise it on the jgroups dev list.

[belaban]

I don't see how passing the sender and response to needMoreResponses() would make your filter stateless: you probably would have to maintain a list of responses, keyed by sender, or update some counter anyway to be able to implement the filter.

[galder.zamarreno]

The moment I have a reponse that matches the following statement, I don't need any more responses. No need to keep list of responses.

lookupSucceeded = (response != null) && !(response instanceof Exception) && !(response instanceof NoHandlerForRPC);

Hence, currently my only state is a boolean arising from that statement and in needMoreResponses(), all I do is return:

return !(lookupSucceeded);

Hence, having response and address in needMoreResponses() would avoid me having to cache that boolean because right now, I only have access to the response in isAcceptable.