ha policy changes and refactoring

[andytaylor]

This patch abstracts the different HA policy configurations into their own classes. This avoids having unrelated configuration elements in the same configuration class or xml chunk.

Because of this the HornetQServerImpl class has been simplified by extracting the different Activations into their own class and moving into the Activation any code that is specific to the actual Activation. This should make it easier to extend or add any new Activation code.

[clebertsuconic]

These are the failures:

est Result (8 failures / ±0)
org.hornetq.byteman.tests.ScaleDownFailoverTest.testScaleDownWhenFirstServerFails
org.hornetq.byteman.tests.ScaleDownFailureTest.testScaleDownWhenRemoteServerIsUnavailable
org.hornetq.byteman.tests.ScaleDownGroupedFailoverTest.testScaleDownWhenFirstServerFails
org.hornetq.byteman.tests.ScaleDownGroupedFailoverTest.testScaleDownWhenFirstServerFails
org.hornetq.byteman.tests.ScaleDownGroupedFailureTest.testScaleDownWhenRemoteServerIsUnavailable
org.hornetq.byteman.tests.ScaleDownGroupedFailureTest.testScaleDownWhenRemoteServerIsUnavailable
org.hornetq.byteman.tests.StartStopDeadlockTest.testDeadlock
org.hornetq.byteman.tests.StompInternalStateTest.testStompProtocolManagerLeak

[clebertsuconic]

you don't have a JIRA for such change?

[andytaylor]

please retest this

[andytaylor]

retest this please

[clebertsuconic]

I have tried to add comments in-line but for some reason my browser was slog with a commit this big:

so, instead let me add the nodes here...

I will add one comment per point as I'm writing as I'm looking though.

here's the first...

ConfigurationImpl::isCheckForLiveServer / setCheckForLiveServer

I think these two methods should be deprecated... and we should make sure that the AS integration is not using it.

How bad would be to delete these deprecated methods?

[clebertsuconic]

ConfigurationUtils::

It's just my taste here.... not strongly against it... but do you really need these many instanceofs? isn't there a way to be more polymorfic and have a method with implementations?

[clebertsuconic]

I liked a lot the refactoring out of Activations (moving Activations as individual classes).. it seems why we didn't do it that way before now

[clebertsuconic]

DefaultsFileConfigurationTest::

There's a big todo fix:

I guess it's best to just remove that? if you intend to fix it I would raise a JIRA... or just leave removed?

[clebertsuconic]

FileConfigurationTest:

Big TODO fix.. same as DefaultFileConfigurationTest::

/* HAPolicy haPolicy = conf.getHAPolicy();
  assertEquals(HAPolicy.POLICY_TYPE.COLOCATED_REPLICATED, haPolicy.getPolicyType());

...

fix it now.. or remove it with a JIRA?

that kind of thing will stay there for years on my experience

[clebertsuconic]

another reason to remove those (my last 2 comments on DefaultFileconfigurationTest and DefaultsFileconfigurationTest is that you added HAPolicyConfigurationTest)

[clebertsuconic]

FailoverTest::testWithoutUsingTheBackup

 waitForRemoteBackupSynchronization(backupServer.getServer());
  //SharedNothingBackupActivation backupActivation =  (SharedNothingBackupActivation) backupServer.getServer().getActivation();
  //assertTrue(backupActivation.waitForBackupSync(10, TimeUnit.SECONDS));

...

  waitForRemoteBackupSynchronization(backupServer.getServer());
  //backupActivation =  (SharedNothingBackupActivation) backupServer.getServer().getActivation();
  //assertTrue(backupActivation.waitForBackupSync(10, TimeUnit.SECONDS));

Can't you just remove these if no longer valid? what's the reason for commenting out?

I didn't get it now.. and I won't certainly get it later if I read this code

[clebertsuconic]

PagingFailoverTest::

Why did you have to remove the waitforbackup on internalTestPage?

I added it there. as far as I remember to avoid intermittent failures

[clebertsuconic]

BindingsClusterTest::

public void setUp() throws Exception
{
//todo fix if needed
super.setUp();
jmsServer1.getHornetQServer().setIdentity("Server 1");
// jmsServer1.getHornetQServer().getConfiguration().getHAPolicy().setFailoverOnServerShutdown(true);
jmsServer2.getHornetQServer().setIdentity("Server 2");
// jmsServer2.getHornetQServer().getConfiguration().getHAPolicy().setFailoverOnServerShutdown(true);
}

Don't we need to set failover on shutdown any longer? if not... then just remove this... if yes.. then why it's passing?

[clebertsuconic]

That's it from me... overall nice job here.. just found these minor things... some of them may not be an issue at all.

Sorry I couldn't add inline comments as it was not working.. it was being very slow on my browser

[andytaylor]

Ive applied all you suggestions and rebased.

Ive removed comments that are not needed and added back in anything that was accidently removed such as the PagingFailoverTest

[mtaylor]

Thanks for updated @andytaylor. Ack from me providing that the tests pass. I'll leave it to @clebertsuconic to merge if he is happy. Cheers.