Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Weight deltas must be absolute deltas #9149

Merged
merged 1 commit into from Jan 6, 2015

Conversation

@s1monw
Copy link
Contributor

commented Jan 5, 2015

In some situations the shard balanceing weight delta becomes negative. Yet,
a negative delta is always treated as well balanced which is wrong. I wasn't
able to reproduce the issue in any way other than useing the real world data
from issue #9023. This commit adds a fix for absolute deltas as well as a base
test class that allows to build tests or simulations from the cat API output.

Closes #9023

@s1monw s1monw added the review label Jan 6, 2015

@dakrone

View changes

src/main/java/org/elasticsearch/cluster/routing/allocation/AllocationService.java Outdated
@@ -224,7 +224,7 @@ private boolean reroute(RoutingAllocation allocation) {

// rebalance
changed |= shardsAllocators.rebalance(allocation);
assert RoutingNodes.assertShardStats(allocation.routingNodes());
// assert RoutingNodes.assertShardStats(allocation.routingNodes());

This comment has been minimized.

Copy link
@dakrone

dakrone Jan 6, 2015

Member

Why is this commented out?

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 6, 2015

Author Contributor

aaah it took too long so I commentd it out

@dakrone

View changes

...ain/java/org/elasticsearch/cluster/routing/allocation/allocator/BalancedShardsAllocator.java Outdated
@@ -186,7 +186,7 @@ public float getShardBalance() {
* </ul>
* <code>weight(node, index) = weight<sub>index</sub>(node, index) + weight<sub>node</sub>(node, index) + weight<sub>primary</sub>(node, index)</code>
*/
public static class WeightFunction {
public class WeightFunction {

This comment has been minimized.

Copy link
@dakrone

dakrone Jan 6, 2015

Member

Extra space here between "public" and "class"

@dakrone

View changes

src/test/java/org/elasticsearch/cluster/routing/allocation/CatAllocationTestBase.java Outdated
IndexMetaData idxMeta = IndexMetaData.builder(idx.name).settings(settings(Version.CURRENT)).numberOfShards(idx.numShards()).numberOfReplicas(idx.numReplicas()).build();
builder.put(idxMeta, false);
IndexRoutingTable.Builder tableBuilder = new IndexRoutingTable.Builder(idx.name).initializeAsRecovery(idxMeta);
Map<Integer, IndexShardRoutingTable> map = new HashMap<>();

This comment has been minimized.

Copy link
@dakrone

dakrone Jan 6, 2015

Member

Can you name this something more descriptive? maybe shardIdToRouting instead of just map

ClusterState clusterState = ClusterState.builder(state).metaData(metaData).routingTable(routingTable).build();
routingTable = strategy.reroute(clusterState).routingTable();
clusterState = ClusterState.builder(clusterState).routingTable(routingTable).build();
while (true) {

This comment has been minimized.

Copy link
@dakrone

dakrone Jan 6, 2015

Member

I think this could use the rebalance function in CatAllocationTestBase? It looks like it's performing the same function

@dakrone

This comment has been minimized.

Copy link
Member

commented Jan 6, 2015

The test for this fails with mvn clean test -Dtests.seed=E920CDE723B09EBE:A5B579D20ADB05EE -Dtests.class=org.elasticsearch.cluster.routing.allocation.BalanceUnbalancedClusterTest -Dtests.method="run" -Dtests.locale=es_PA -Dtests.timezone=America/Resolute -Dtests.processors=4

Output:

  1> Throwable:
  1> java.lang.AssertionError: 3.8157892 lt 7.5526314 but was expected to be gte
  1>     __randomizedtesting.SeedInfo.seed([E920CDE723B09EBE:A5B579D20ADB05EE]:0)
  1>     org.elasticsearch.cluster.routing.allocation.allocator.BalancedShardsAllocator$Balancer.absDelta(BalancedShardsAllocator.java:352)
  1>     org.elasticsearch.cluster.routing.allocation.allocator.BalancedShardsAllocator$Balancer.balance(BalancedShardsAllocator.java:402)
  1>     org.elasticsearch.cluster.routing.allocation.allocator.BalancedShardsAllocator.rebalance(BalancedShardsAllocator.java:124)
  1>     org.elasticsearch.cluster.routing.allocation.allocator.ShardsAllocators.rebalance(ShardsAllocators.java:80)
  1>     org.elasticsearch.cluster.routing.allocation.AllocationService.reroute(AllocationService.java:226)
  1>     org.elasticsearch.cluster.routing.allocation.AllocationService.reroute(AllocationService.java:160)
  1>     org.elasticsearch.cluster.routing.allocation.AllocationService.reroute(AllocationService.java:146)
  1>     org.elasticsearch.cluster.routing.allocation.CatAllocationTestBase.rebalance(CatAllocationTestBase.java:137)
  1>     org.elasticsearch.cluster.routing.allocation.CatAllocationTestBase.run(CatAllocationTestBase.java:123)
  1>     [...sun.*, java.lang.reflect.*, com.carrotsearch.randomizedtesting.*]
  1>     org.apache.lucene.util.TestRuleSetupTeardownChained$1.evaluate(TestRuleSetupTeardownChained.java:50)
  1>     org.apache.lucene.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:46)
  1>     [...com.carrotsearch.randomizedtesting.*]

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Jan 6, 2015

@dakrone I pushed a new commit fixing the test failures...

@dakrone

This comment has been minimized.

Copy link
Member

commented Jan 6, 2015

LGTM

[ALLOCATION] Weight deltas must be absolute deltas
In some situations the shard balanceing weight delta becomes negative. Yet,
a negative delta is always treated as `well balanced` which is wrong. I wasn't
able to reproduce the issue in any way other than useing the real world data
from issue #9023. This commit adds a fix for absolute deltas as well as a base
test class that allows to build tests or simulations from the cat API output.

Closes #9023

@s1monw s1monw force-pushed the s1monw:issue/9023 branch to 4900f52 Jan 6, 2015

@s1monw s1monw merged commit 4900f52 into elastic:master Jan 6, 2015

1 check passed

CLA Commit author is a member of Elasticsearch
Details

@clintongormley clintongormley changed the title [ALLOCATION] Weight deltas must be absolute deltas Allocation: Weight deltas must be absolute deltas Feb 10, 2015

@clintongormley clintongormley changed the title Allocation: Weight deltas must be absolute deltas Weight deltas must be absolute deltas Jun 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.