ISPN-7461 Cache is not rebalanced on merge

[jurakp]

https://issues.jboss.org/browse/ISPN-7461

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1421404

Removed problematic if block.

[ghost]

Can one of the admins verify this patch?

[wburns]

@danberindei can you take a look at this? It seems this might be a good one for 9.0, wdyt?

[rvansa]

@wburns It's meant for 5.2 :) I don't think we can guarantee much after merge - merge without partition handling is scarcely tested (if at all). A proper solution is related to AP mode - diverged entries merge (I don't know if @ryanemerson is about to work on that given that he has worked on the consistency checker).

[ryanemerson]

@rvansa @wburns The plan is for Automatic Merging with various merge strategies to be implemented for 9.1. This is currently a WIP, but you can see the rough design docs here.

[wburns]

@rvansa Oh I know that we can't guarantee anything after merge in regards to data. I was more asking because this change only has to do with the topology updates.

[danberindei]

Oops, I missed this one... for some reason I'm showing up as a reviewer here but not in the PR list :)

I'm don't think this is the right approach. DefaultRebalancePolicy.updateCacheStatus() is called from lots of places, and I'm afraid without this check it could get into an infinite loop. I would rather fix isBalanced(), making it more like the check in startRebalance():

     ConsistentHash updatedMembersCH = chFactory.updateMembers(currentCH, newMembers);
     ConsistentHash balancedCH = chFactory.rebalance(updatedMembersCH);
     return balancedCH.equals(currentCH);

[jurakp]

I have rewritten it based on @danberindei comment. The main problem was encapsulation of cache status map. I did not want to modify ClusterTopologyManagerImpl just for test, therefore I use reflection to get that map in TestingUtil.

[danberindei]

Just one minor comment

[danberindei]

If currentCH == null, updateMembers(currentCH, ...) would throw a NPE. So we should either check for null before, or remove the check here. I don't think it's possible to have a CacheTopology with a currentCH == null, is it?

[jurakp]

@danberindei You were right. I have removed unnecessary null check.

[danberindei]

Integrated, thanks Petr!