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

Expand ClusterInfo to provide min / max disk usage for allocation decider #13163

Merged
merged 1 commit into from Aug 28, 2015

Conversation

s1monw
Copy link
Contributor

@s1monw s1monw commented Aug 27, 2015

Today we sum up the disk usage for the allocation decider which is broken since
we don't stripe across multiple data paths. Each shard has it's own private path
now but the allocation deciders still treat all paths as one big disk. This commit
adds allows allocation deciders to access the least used and most used path to make
better allocation decisions upon canRemain and canAllocate calls.

Yet, this commit doesn't fix all the issues since we still can't tell which shard
can remain and which can't. This problem is out of scope in this commit and will be solved
in a followup commit.

Relates to #13106

@@ -77,7 +83,7 @@ public long getUsedBytes() {

@Override
public String toString() {
return "[" + nodeId + "][" + nodeName + "] free: " + new ByteSizeValue(getFreeBytes()) +
return "[" + nodeId + "][" + nodeName + "][" + path + "] free: " + new ByteSizeValue(getFreeBytes()) +
Copy link
Member

Choose a reason for hiding this comment

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

nice.

@dakrone
Copy link
Member

dakrone commented Aug 27, 2015

LGTM, but worth adding something mentioning it in the documentation?

@s1monw
Copy link
Contributor Author

s1monw commented Aug 27, 2015

LGTM, but worth adding something mentioning it in the documentation?

@dakrone you mean the main documenation or javadocs?

@dakrone
Copy link
Member

dakrone commented Aug 27, 2015

The main documentation is mostly what I meant (javadocs never hurt though! ;) )

@s1monw
Copy link
Contributor Author

s1monw commented Aug 27, 2015

honestly I don't know what I should add there... I am just fixing a bug here not adding functionality?

@s1monw s1monw added the >bug label Aug 27, 2015
@dakrone
Copy link
Member

dakrone commented Aug 27, 2015

honestly I don't know what I should add there... I am just fixing a
bug here not adding functionality?

Maybe something like this:

"Prior to 2.0.0, when using multiple data paths, the disk threshold
decider only factored in the usage across all data paths (if you had two
data paths, one with 50b out of 100b free (50% used) and another with
40b out of 50b free (80% used) it would see the node's disk usage as 90b
out of 150b). In 2.0.0, the minimum and maximum disk usages are tracked
separately."

I think it's mostly for people upgrading from 1.x to 2.x to understand a
change in behavior.

@dakrone
Copy link
Member

dakrone commented Aug 27, 2015

It's optional though, it doesn't need to block this PR going in

@s1monw
Copy link
Contributor Author

s1monw commented Aug 27, 2015

@dakrone yeah I am still trying to digest if people need to know about this impl detail?

@dakrone
Copy link
Member

dakrone commented Aug 27, 2015

Usually I would say no, but because this can make the difference between "why were my shards allocated in 1.7 but not in 2.0?" I think it is a useful thing to add

@dakrone dakrone changed the title Expand ClusterInfo to provide min / max disk usage forn allocation decider Expand ClusterInfo to provide min / max disk usage for allocation decider Aug 27, 2015
@s1monw
Copy link
Contributor Author

s1monw commented Aug 27, 2015

Usually I would say no, but because this can make the difference between "why were my shards allocated in 1.7 but not in 2.0?" I think it is a useful thing to add

I will make sure I add this before I close #13106

@dakrone
Copy link
Member

dakrone commented Aug 27, 2015

I will make sure I add this before I close #13106

Sounds good to me! Thanks!

…cider

Today we sum up the disk usage for the allocation decider which is broken since
we don't stripe across multiple data paths. Each shard has it's own private path
now but the allocation deciders still treat all paths as one big disk. This commit
adds allows allocation deciders to access the least used and most used path to make
better allocation decidsions upon canRemain and canAllocate calls.

Yet, this commit doesn't fix all the issues since we still can't tell which shard
can remain and which can't. This problem is out of scope in this commit and will be solved
in a followup commit.

Relates to elastic#13106
@s1monw s1monw merged commit 0c71328 into elastic:master Aug 28, 2015
@clintongormley
Copy link

@s1monw i assume you're still planning on backporting this to 2.0? I fixed the version label for master.

@s1monw s1monw deleted the min_max_disk_space branch August 31, 2015 13:34
@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
Labels
>bug :Distributed/Allocation All issues relating to the decision making around placing a shard (both master logic & on the nodes) v2.0.0-beta2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants