Skip to content
This repository has been archived by the owner on Feb 1, 2021. It is now read-only.

Add spread strategy and make it the default #458

Merged
merged 1 commit into from
Mar 10, 2015
Merged

Add spread strategy and make it the default #458

merged 1 commit into from
Mar 10, 2015

Conversation

vieux
Copy link
Contributor

@vieux vieux commented Mar 7, 2015

Signed-off-by: Victor Vieux [email protected]

@vieux vieux added this to the 0.2.0 milestone Mar 7, 2015
@vieux vieux self-assigned this Mar 7, 2015
@vieux
Copy link
Contributor Author

vieux commented Mar 7, 2015

@jhamrick & @aluzzardi can you please take a look ?

assert.NoError(t, err)
assert.NoError(t, AddContainer(node3, createContainer("c3", config)))

// check that it ends up on the same node as the 3G
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this comment should be "same node as the 2G", otherwise it doesn't match the code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I updated.

@jhamrick
Copy link
Contributor

jhamrick commented Mar 7, 2015

👍 thanks, this looks awesome!

@chanwit
Copy link
Contributor

chanwit commented Mar 7, 2015

@vieux is spread the replacement of the balanced proposed a while ago?
if so, great to see it's made as the default one !

return nil, err
}

// sort by highest weight
Copy link
Contributor

Choose a reason for hiding this comment

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

this comment seems contradicting (sort.Sort(sort.Reverse(foo)) sorts in reverse order, which would be by lowest weight)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's the same comment as in your PR https://github.com/docker/swarm/pull/398/files

If it's not clear, even for you, we have an issue :D

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh right. sort.Sort sorts low to high... sorry, forgot my daily dose of ☕ this morning.

@bacongobbler
Copy link
Contributor

👍 considering that this scheduling strategy has been proposed at least 3 times now it sounds like users want this feature :)

@@ -28,3 +31,39 @@ func (n weightedNodeList) Less(i, j int) bool {

return ip.Weight < jp.Weight
}

func weighNodes(config *dockerclient.ContainerConfig, nodes []cluster.Node) (weightedNodeList, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: weightNodes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

:D

@aluzzardi
Copy link
Contributor

LGTM

aluzzardi added a commit that referenced this pull request Mar 10, 2015
Add spread strategy and make it the default
@aluzzardi aluzzardi merged commit 70fb3d5 into docker-archive:master Mar 10, 2015
@aluzzardi aluzzardi deleted the reverse branch March 10, 2015 23:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants