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.

Sign up for GitHub

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

Jump to bottom

Take initializing shards into consideration during awareness allocation #12551

Merged
merged 1 commit into from Sep 11, 2015
Merged

Take initializing shards into consideration during awareness allocation #12551

merged 1 commit into from Sep 11, 2015

Conversation

masaruh
Copy link
Contributor

@masaruh masaruh commented Jul 30, 2015

It makes decision consistent.
Fixes #12522

@dakrone
Copy link
Member

dakrone commented Aug 13, 2015

This looks good to me, but I think @s1monw should take a look also just to make sure.

Also, not sure about where this should go, @clintongormley what do you think?

s1monw
s1monw reviewed Sep 10, 2015
View reviewed changes
core/src/test/java/org/elasticsearch/cluster/routing/allocation/AwarenessAllocationTests.java
@@ -763,7 +763,7 @@ public void fullAwareness3() {
}

@Test
public void testUnbalancedZones() {
public void testUnbalancedZones1() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we get a better name for this?

@s1monw
Copy link
Contributor

s1monw commented Sep 10, 2015

left trivial comments - LGTM though

@s1monw
Copy link
Contributor

s1monw commented Sep 10, 2015

I think we should let it bake on master and 2.x and if it's stable port to 2.0?

@masaruh masaruh merged commit 5ae00a6 into elastic:master Sep 11, 2015
@masaruh
Copy link
Contributor Author

masaruh commented Sep 11, 2015

Thanks. Updated test names.
Pushed it to master, 2.x and 1.7 for now.

@s1monw
Copy link
Contributor

s1monw commented Sep 11, 2015

if you push to 1.7 you also need to push to 2.0 otherwise this makes no sense

@masaruh
Copy link
Contributor Author

masaruh commented Sep 11, 2015

oh... pushed to 2.0 as well.

bleskes
bleskes reviewed Sep 11, 2015
View reviewed changes
...in/java/org/elasticsearch/cluster/routing/allocation/decider/AwarenessAllocationDecider.java
@@ -188,7 +188,7 @@ private Decision underCapacity(ShardRouting shardRouting, RoutingNode node, Rout
if (assignedShard.relocating()) {
RoutingNode relocationNode = allocation.routingNodes().node(assignedShard.relocatingNodeId());
shardPerAttribute.addTo(relocationNode.node().attributes().get(awarenessAttribute), 1);
} else if (assignedShard.started()) {
} else if (assignedShard.started() || assignedShard.initializing()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means we are counting target relocation shards twice. Once with the source shards, and once here. We can remove the assignedShard.relocating part yet.

@bleskes
Copy link
Contributor

bleskes commented Sep 11, 2015

Late to the party, but I think there is a problem with this fix. Left a comment on it. The fact we didn't catch it also means our tests are not strong enough. @masaruh let me know if you want to continue here, o.w. I'll pick it up.

@masaruh masaruh mentioned this pull request Sep 11, 2015
Merged
@masaruh
Copy link
Contributor Author

masaruh commented Sep 11, 2015

@bleskes you are right... Created #13512 (the test fails without the fix).

masaruh added a commit to masaruh/elasticsearch that referenced this pull request Sep 11, 2015
@masaruh
Take relocating shard into consideration during awareness allocation
6a9ef99 
Previous fix elastic#12551 counted twice for relocating shard (source and target).
Fix it to consider only target node.
masaruh added a commit that referenced this pull request Sep 11, 2015
@masaruh
Take relocating shard into consideration during awareness allocation
be1c8a8 
Previous fix #12551 counted twice for relocating shard (source and target).
Fix it to consider only target node.
masaruh added a commit that referenced this pull request Sep 11, 2015
@masaruh
Take relocating shard into consideration during awareness allocation
c30e7fb 
Previous fix #12551 counted twice for relocating shard (source and target).
Fix it to consider only target node.
masaruh added a commit that referenced this pull request Sep 11, 2015
@masaruh
Take relocating shard into consideration during awareness allocation
f4145d7 
Previous fix #12551 counted twice for relocating shard (source and target).
Fix it to consider only target node.
@clintongormley clintongormley mentioned this pull request Nov 21, 2015
Closed
@lcawl lcawl added :Distributed/Distributed A catch all label for anything in the Distributed Area. If you aren't sure, use this one. and removed :Allocation labels Feb 13, 2018
@clintongormley clintongormley added :Distributed/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) and removed :Distributed/Distributed A catch all label for anything in the Distributed Area. If you aren't sure, use this one. labels Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
>bug :Distributed/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) v1.7.2 v2.0.0-beta2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@masaruh @dakrone @s1monw @bleskes @clintongormley @lcawl