JBossDeveloper

Yes fine, but let's not overcomplicate things :)

Clebert,

I've just taken a look at your check in. Here are several questions:

1. ServerConnectionFactoryEndpoint sends a view update to ALL active connections returned by the ConnectionManager instance. Why does it send it to all? Didn't you mention that this has to be sent on a "filtered" list of connections? This JIRA issue (http://jira.jboss.com/jira/browse/JBMESSAGING-759) is about this.

2. Why the code that sends the view update (currently hosted by ServerConnectionFactoryEndpoint) is not located at ConnectionManager level? Seems more appropriate to be there...

3. Why do you keep a different "activeConnections" set at ConnectionManger level? Speed reasons? You got all the information you need in the "endpoints" map already. Keep in mind that cluster view update events are very rare events, considering the time scale at which messaging operates. Why would you need to optimize that, risking cache desynchronization?

4. What's the difference between a LoadBalancingPolicy and a LoadBalancingFactory? Why do you configure a ConnectionFactory service with the latter instead of the former?

5. What is this:

// FailoverNodeID is not on the map, that means the ConnectionFactory was updated
 // by another connection in another server.. So we will have to guess the failoverID
 // by numeric order. Case we guessed the new server wrongly we will have to rely on
 // redirect from failover

I've finally managed to look at the new code.

A few questions:

1. The update information is sent from server to client in a ConnectionFactoryUpdateMessage instance. This needs to be wrapped in a MessagingMarshallable, otherwise we can't version it.

2. ServerConnectionFactoryEndpoint::updateclusteredClients() is currently only called when a connection factory is deployed / undeployed. It is not called when a node joins or leaves the cluster. It needs to be called in this case too.
Also you need to add tests to check that the update occurs in this situation.

3. Why have you made MessageCallbackHandler and ConnetionFactoryCallbackHandler implement the same (new) interface (CallbackHandler) ? This seems redundant since they are never called polymorphically anyway.

4. I was unsure why init() was being called on the ClientClusteredConnectionFactoryDelegate every time a failover view change occurred...

Cheers

Clebert-

I just tried to deploy the scoped sar in JBAS and it fails, complaining that it cannot find a property editor for ConnectionFactory::setLoadBalancingFactory.

Looking at the code, the method setLoadBalancingFactory() takes a LoadBalancingFactory, but the MBean method takes a String.

I have fixed this, so I can get on with my work. You may want to review my fix since it was very quick.

This tells me one thing, you added an MBean attribute without adding a corresponding test for it.

All new functionality needs to have unit tests, so please could you add a test.

Cheers.

"clebert.suconic@jboss.com" wrote:



Are you sure about this? I have testcases about this... a testcase that gets the ConnectionFactory updated when a new server joins / leaves the cluster.

I didn't want MessageCallbackHandler to also take care of ConnectionFactories... I thought the dependency would be wrong.

"clebert.suconic@jboss.com" wrote:
ConnectionFactoryJNDIMapper::onReplicationChange is calling updating ServerCFEndpoing::updateClusteredClients...

onReplicateChange is also setting the new LoadBalancing when the data is being changed.

ClusterViewUpdateTest::testUpdateConnectionFactory is testing updates on CFs... it kills the server and verifies if the connectinFactory was changed.

Test was passing for me on Friday.

I didn't want MessageCallbackHandler to also take care of ConnectionFactories... I thought the dependency would be wrong.

If the client view is being updated, as you say, when a node crashes, then the only other possibility AFAICT is that the connection factory deploy/undeploy even is being triggered when a node crashes.

Can you explain how this happens?