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

Add support for multiple disks resources #314

Merged
merged 3 commits into from
Sep 10, 2017
Merged

Add support for multiple disks resources #314

merged 3 commits into from
Sep 10, 2017

Conversation

everpcpc
Copy link
Contributor

@everpcpc everpcpc commented Sep 5, 2017

Disk source should also be checked. Otherwise it will cause a mistaken resource comparison when using multiple disks.

@coveralls
Copy link

coveralls commented Sep 5, 2017

Coverage Status

Coverage decreased (-0.2%) to 52.448% when pulling 8dc5789 on everpcpc:disk into 464c8d6 on mesos:master.

@jdef
Copy link
Contributor

jdef commented Sep 6, 2017

Thanks for the PR! Would you mind fleshing out TestResources_PersistentVolumes or else writing a new unit test to exercise this new code?

@everpcpc everpcpc changed the title Also check source when comparing disks Add support for multiple disks resources Sep 8, 2017
@coveralls
Copy link

coveralls commented Sep 8, 2017

Coverage Status

Coverage decreased (-0.1%) to 52.49% when pulling 7cdc264 on everpcpc:disk into 464c8d6 on mesos:master.

@everpcpc
Copy link
Contributor Author

everpcpc commented Sep 8, 2017

Hi, I've put the tests in TestResources_Equivalent.

Copy link
Contributor

@jdef jdef left a comment

Choose a reason for hiding this comment

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

Thanks for the additional changes! Couple of minor comments.

buf.WriteString(m.GetRoot())
}
default:
buf.WriteString("(ROOT:")
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 rid of ROOT: output here? I'd like for the output to be mostly consistent with https://github.com/apache/mesos/blob/1.2.x/src/common/resources.cpp#L1922

@@ -72,7 +72,7 @@ func Reservation(ri *mesos.Resource_ReservationInfo) Opt {
}
}

func Disk(persistenceID, containerPath string) Opt {
func Disk(persistenceID, containerPath, source string, sourceType mesos.Resource_DiskInfo_Source_Type) Opt {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: would rather have left the params for Disk alone and created a new DiskWithSource func w/ the extended params. Callers of Disk(x, y, "", 0) shouldn't actually need to bother with the additional params.

@coveralls
Copy link

coveralls commented Sep 9, 2017

Coverage Status

Coverage decreased (-0.1%) to 52.51% when pulling 46ddaa4 on everpcpc:disk into 464c8d6 on mesos:master.

@jdef
Copy link
Contributor

jdef commented Sep 10, 2017

thanks, lgtm

@jdef jdef merged commit 479884c into mesos:master Sep 10, 2017
@everpcpc everpcpc deleted the disk branch September 10, 2017 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants