Simplify BalanceUnbalancedClusterTest

[rjernst]

This takes 10 seconds or more, while other allocation tests are almost instantaneous. Can we simplify this? It looks like it tries to do a basic allocation (5 shards, 1 replica) of a new index when a ton of indexes already exist on just 4 nodes. Perhaps we could test similar circumstances without thousands of shards? Alternatively, we could just make this an integration test (leave the impl, but rename to IT). It doesn't really seem like a unit test as it is now.

Also, as a side note, this test is the only user of CatAllocationTestCase. Perhaps we can also eliminate this abstraction and just test directly (eliminating the zipped shard state)? @s1monw do you have any thoughts here?

[DaveCTurner]

I looked into trying to make this test be smaller without being less useful.

This test was introduced in #9149 as a fix for #9023. I'll admit I don't completely follow what happened in #9149 - the words indicate there was an unexpected sign inversion, but it looks like a bigger change; the key seems to be the following:

diff --git a/src/main/java/org/elasticsearch/cluster/routing/allocation/allocator/BalancedShardsAllocator.java b/src/main/java/org/elasticsearch/cluster/routing/allocation/allocator/BalancedShardsAllocator.java
index 5484498237..7c75b2a061 100644
--- a/src/main/java/org/elasticsearch/cluster/routing/allocation/allocator/BalancedShardsAllocator.java
+++ b/src/main/java/org/elasticsearch/cluster/routing/allocation/allocator/BalancedShardsAllocator.java
@@ -393,8 +393,7 @@ public class BalancedShardsAllocator extends AbstractComponent implements Shards
                             final ModelNode maxNode = modelNodes[highIdx];
                             advance_range:
                             if (maxNode.numShards(index) > 0) {
-                                float delta = weights[highIdx] - weights[lowIdx];
-                                delta = lessThan(delta, threshold) ? delta : sorter.weight(Operation.THRESHOLD_CHECK, maxNode) - sorter.weight(Operation.THRESHOLD_CHECK, minNode);
+                                float delta = absDelta(weights[lowIdx], weights[highIdx]);
                                 if (lessThan(delta, threshold)) {
                                     if (lowIdx > 0 && highIdx-1 > 0 // is there a chance for a higher delta?
                                         && (weights[highIdx-1] - weights[0] > threshold) // check if we need to break at all

My best guess is that the bug was that sorter.weight(Operation.THRESHOLD_CHECK, maxNode) was less than sorter.weight(Operation.THRESHOLD_CHECK, minNode). However, this condition is long-gone, and this code is now quite different from how it was then, so I cannot see a way to make this test fail meaningfully.

Additionally, the only real failure of this test that I can find is a PR build (build-1463180427010/t/20161125152515-B0401F02) in November 2016 (in which lots of other allocation tests also failed).

@s1monw can you suggest a way forward here? Perhaps one of the following:

• Find a way to make it fail meaningfully again (so that I can work on finding a smaller input that triggers the same failure).
• Move this to a test suite more appropriate for slower tests.
• Remove it entirely.

[elasticmachine]

Pinging @elastic/es-distributed

[DaveCTurner]

I looked at this again recently. This test now runs in ~1sec on my laptop (after warming up) and CI reports it taking ~3seconds. That's still a bit much, but nothing like as bad as the 10s reported above.

I think we could perhaps replace it with a more generic test that builds up a large-ish cluster with a large-ish number of indices and asserts that the cluster is appropriately balanced at the end. I say this tentatively because it'll be a bit delicate to get an assertion that's usefully strong without being false.

[DaveCTurner]

We discussed this in today's team meeting and decided to close this issue. The test is acceptably fast these days and a number of us have all thought about how to generalise or improve this test without any success.