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

DiskThresholdDecider#remain(...) should take shards relocating away into account #8659

Conversation

martijnvg
Copy link
Member

Let the disk threshold decider take into account shards moving away from a node in order to determine if a shard can remain.

By taking this into account we can prevent that we move too many shards away than is necessary.

PR for #8538


DiskThresholdDecider diskThresholdDecider = new DiskThresholdDecider(diskSettings);
MetaData metaData = MetaData.builder()
.put(IndexMetaData.builder("test").numberOfShards(1).numberOfReplicas(1))
Copy link
Member

Choose a reason for hiding this comment

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

You put [test][0][p] and [test][1][p], but then create metadata with 1 primary shard and one replica, can you change it here or in the shardSizes map?

@dakrone
Copy link
Member

dakrone commented Nov 25, 2014

@martijnvg left a couple of comments

@dakrone
Copy link
Member

dakrone commented Nov 25, 2014

Also, 1.3.x doesn't include the take-relocations-into-account feature, so this may be pretty tough to backport without backporting the original feature, not sure if this is something we should stick in just 1.4.x and up.

@martijnvg martijnvg removed the v1.3.6 label Nov 25, 2014
@martijnvg
Copy link
Member Author

@dakrone Good point, didn't realise that. I removed the 1.3.6 version label. I'll update the PR.

@@ -223,20 +223,28 @@ public DiskThresholdDecider(Settings settings, NodeSettingsService nodeSettingsS
/**
* Returns the size of all shards that are currently being relocated to
* the node, but may not be finished transfering yet.
*
* If includeShardsMovingAway is set then the size of shards moving away is subtracted from the total size
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 name this subtractShardsMovingAway ? it's clear what it means

@s1monw
Copy link
Contributor

s1monw commented Nov 25, 2014

left one comment - thanks mvg for getting this in!!

@martijnvg
Copy link
Member Author

@dakrone @s1monw Thanks for the feedback, I've updated the PR.

@@ -223,20 +223,28 @@ public DiskThresholdDecider(Settings settings, NodeSettingsService nodeSettingsS
/**
* Returns the size of all shards that are currently being relocated to
* the node, but may not be finished transfering yet.
*
* If subtractShardsMovingAwayRen is set then the size of shards moving away is subtracted from the total size
Copy link
Member

Choose a reason for hiding this comment

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

subtractShardsMovingAwayRen -> subtractShardsMovingAway

@dakrone
Copy link
Member

dakrone commented Nov 26, 2014

LGTM, left one comment about a misspelling

@martijnvg
Copy link
Member Author

oops, I'll change that.

On 26 November 2014 at 09:36, Lee Hinman notifications@github.com wrote:

LGTM, left one comment about a misspelling


Reply to this email directly or view it on GitHub
#8659 (comment)
.

Met vriendelijke groet,

Martijn van Groningen

…away from a node in order to determine if a shard can remain.

By taking this into account we can prevent that we move too many shards away than is necessary.

Closes elastic#8538
Closes elastic#8659
@martijnvg martijnvg force-pushed the improvements/disk_threshold_decider_take_shards_relocating_away_into_account branch from f9fc09e to c70e09e Compare November 26, 2014 08:55
martijnvg added a commit to martijnvg/elasticsearch that referenced this pull request Nov 26, 2014
…away from a node in order to determine if a shard can remain.

By taking this into account we can prevent that we move too many shards away than is necessary.

Closes elastic#8538
Closes elastic#8659
@martijnvg martijnvg merged commit c70e09e into elastic:1.x Nov 26, 2014
martijnvg added a commit that referenced this pull request Nov 26, 2014
…away from a node in order to determine if a shard can remain.

By taking this into account we can prevent that we move too many shards away than is necessary.

Closes #8538
Closes #8659
@clintongormley clintongormley changed the title Core: DiskThresholdDecider#remain(...) should take shards relocating away into account Allocation: DiskThresholdDecider#remain(...) should take shards relocating away into account Nov 26, 2014
@martijnvg martijnvg deleted the improvements/disk_threshold_decider_take_shards_relocating_away_into_account branch May 18, 2015 23:28
@clintongormley clintongormley changed the title Allocation: DiskThresholdDecider#remain(...) should take shards relocating away into account DiskThresholdDecider#remain(...) should take shards relocating away into account Jun 6, 2015
mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015
…away from a node in order to determine if a shard can remain.

By taking this into account we can prevent that we move too many shards away than is necessary.

Closes elastic#8538
Closes elastic#8659
@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed/Distributed A catch all label for anything in the Distributed Area. If you aren't sure, use this one. >enhancement v1.4.1 v1.5.0 v2.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants