You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Guangya Liu <gy...@gmail.com> on 2016/01/02 02:16:06 UTC
Re: Review Request 41772: WIP: Added helper function to flatten
resources.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/
-----------------------------------------------------------
(Updated 一月 2, 2016, 1:16 a.m.)
Review request for mesos and Klaus Ma.
Repository: mesos
Description
-------
Added two new helper functions to flatten resources.
1) Flatten reserved resources.
2) Flatten allocation slack revocable resources.
Diffs (updated)
-----
include/mesos/resources.hpp 9c8bfb0e7e1e40ca99b4cc75079e5b950630243d
include/mesos/v1/resources.hpp db87c992e2ec466cb1d4d17775bf6423a2fb15aa
src/common/resources.cpp b9d31aef8babee212374e352c57fadbff02167f3
src/tests/resources_tests.cpp e4a3435adc14f3b6b278b32348a6991543d5a320
src/v1/resources.cpp 0c0eee6b3b7ae3254d7dc7b06187855c9b873764
Diff: https://reviews.apache.org/r/41772/diff/
Testing
-------
make
make check
Thanks,
Guangya Liu
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/
-----------------------------------------------------------
(Updated 三月 12, 2016, 2:46 a.m.)
Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
Bugs: MESOS-4267
https://issues.apache.org/jira/browse/MESOS-4267
Repository: mesos
Description
-------
Added helper function to flatten resources.
Diffs
-----
include/mesos/resources.hpp 388c4f383b77be490cb2debb64a734c6d6e4a176
include/mesos/v1/resources.hpp 64ad8bf08230aeaa173325364c91b765f091210e
src/common/resources.cpp cf0707209143084d86aff5e4427846f461479c54
src/tests/resources_tests.cpp 5357275833383bbff055689c8c1d0fa59791f2d0
src/v1/resources.cpp 296c4c2392384a2fcc4f2c1843980ff97e71114d
Diff: https://reviews.apache.org/r/41772/diff/
Testing
-------
make
make check
GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ResourcesOperationTest.*" --verbose
Thanks,
Guangya Liu
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/
-----------------------------------------------------------
(Updated 三月 11, 2016, 9:10 a.m.)
Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
Bugs: MESOS-4267
https://issues.apache.org/jira/browse/MESOS-4267
Repository: mesos
Description (updated)
-------
Added helper function to flatten resources.
Diffs (updated)
-----
include/mesos/resources.hpp 388c4f383b77be490cb2debb64a734c6d6e4a176
include/mesos/v1/resources.hpp 64ad8bf08230aeaa173325364c91b765f091210e
src/common/resources.cpp cf0707209143084d86aff5e4427846f461479c54
src/tests/resources_tests.cpp 5357275833383bbff055689c8c1d0fa59791f2d0
src/v1/resources.cpp 296c4c2392384a2fcc4f2c1843980ff97e71114d
Diff: https://reviews.apache.org/r/41772/diff/
Testing
-------
make
make check
GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ResourcesOperationTest.*" --verbose
Thanks,
Guangya Liu
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/
-----------------------------------------------------------
(Updated 二月 29, 2016, 7:08 a.m.)
Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
Bugs: MESOS-4267
https://issues.apache.org/jira/browse/MESOS-4267
Repository: mesos
Description
-------
Added helper function to flatten allocation slack resources.
Diffs
-----
include/mesos/resources.hpp cc8fef9470d779078aa408ed03e747e5a492deaa
include/mesos/v1/resources.hpp f4892977f8d7b0439db6e9cf7921334f606a496c
src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a
src/tests/resources_tests.cpp b42610f1bf8eacfd7bf388d351f8745f1d96f666
src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc
Diff: https://reviews.apache.org/r/41772/diff/
Testing
-------
make
make check
GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ResourcesOperationTest.*" --verbose
Thanks,
Guangya Liu
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/
-----------------------------------------------------------
(Updated 一月 23, 2016, 2:59 a.m.)
Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
Bugs: MESOS-4267
https://issues.apache.org/jira/browse/MESOS-4267
Repository: mesos
Description
-------
Added helper function to flatten allocation slack resources.
Diffs (updated)
-----
include/mesos/resources.hpp cc8fef9470d779078aa408ed03e747e5a492deaa
include/mesos/v1/resources.hpp f4892977f8d7b0439db6e9cf7921334f606a496c
src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a
src/tests/resources_tests.cpp b42610f1bf8eacfd7bf388d351f8745f1d96f666
src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc
Diff: https://reviews.apache.org/r/41772/diff/
Testing
-------
make
make check
GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ResourcesOperationTest.*" --verbose
Thanks,
Guangya Liu
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/
-----------------------------------------------------------
(Updated 一月 23, 2016, 2:17 a.m.)
Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
Bugs: MESOS-4267
https://issues.apache.org/jira/browse/MESOS-4267
Repository: mesos
Description (updated)
-------
Added helper function to flatten allocation slack resources.
Diffs (updated)
-----
include/mesos/resources.hpp cc8fef9470d779078aa408ed03e747e5a492deaa
include/mesos/v1/resources.hpp f4892977f8d7b0439db6e9cf7921334f606a496c
src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a
src/tests/resources_tests.cpp b42610f1bf8eacfd7bf388d351f8745f1d96f666
src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc
Diff: https://reviews.apache.org/r/41772/diff/
Testing
-------
make
make check
GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ResourcesOperationTest.*" --verbose
Thanks,
Guangya Liu
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/
-----------------------------------------------------------
(Updated 一月 20, 2016, 3:40 a.m.)
Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
Bugs: MESOS-4267
https://issues.apache.org/jira/browse/MESOS-4267
Repository: mesos
Description
-------
Added helper function to flatten resources.
Diffs (updated)
-----
include/mesos/resources.hpp cc8fef9470d779078aa408ed03e747e5a492deaa
include/mesos/v1/resources.hpp f4892977f8d7b0439db6e9cf7921334f606a496c
src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a
src/tests/resources_tests.cpp b42610f1bf8eacfd7bf388d351f8745f1d96f666
src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc
Diff: https://reviews.apache.org/r/41772/diff/
Testing
-------
make
make check
GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ResourcesOperationTest.*" --verbose
Thanks,
Guangya Liu
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Klaus Ma <kl...@gmail.com>.
> On Jan. 20, 2016, 6:18 a.m., Joseph Wu wrote:
> > src/common/resources.cpp, line 880
> > <https://reviews.apache.org/r/41772/diff/6/?file=1199181#file1199181line880>
> >
> > It should be fine to just name this `flatten`.
> >
> > You should also consider changing the parameter type from `RevocableInfo::Type` to just `RevocableInfo`. Supplying the whole `RevocableInfo` will save us another tweak to the helpers if we change the revocable protobufs again.
>
> Guangya Liu wrote:
> I think that we cannot rename it to `flatten` as the compiler will not know what does `flatten()` mean.
Regarding from `RevocableInfo::Type` to `RevocableInfo`, do you mean change `RevocableInfo` from `message` to `enum`? I think it's better to define const in `Resources` instead of changing `RevocableInfo`'s type, e.g. `static const Resource::RevocableInfo::Type Resources::ALLOCATION_SLACK = Resource::RevocableInfo::Type::ALLOCATION_SLACK`.
Regarding `flatten`, there're two options to me:
* Option 1: add `RevocableInfo::Type` to as 3rd parameter in current `flatten` with `None()` as default vaule, and a new helper function to accept `RevocableInfo::Type` only without default value. The sample code is:
```
Resources flatten(string role = "*", Option<ReservationInfo> resveration = None(), Option<RevocableInfo::Type> revocable = None()) {
...
if (revocable.isSome()) {
flatten(revocable.get());
} else {
// clear revocable info.
}
}
Resources flatten(RevocableInfo::Type revocable) {
foreach resources {
resource.mutable_revocable()->set_type(revocable);
}
}
```
* Option 2: add new `flatten` functions but did not provide default value; `None()` means clear the `RevocableInfo::Type`.
I perfer to Option 1 :). Any comments?
- Klaus
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/#review115249
-----------------------------------------------------------
On Jan. 20, 2016, 11:40 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41772/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2016, 11:40 a.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
>
>
> Bugs: MESOS-4267
> https://issues.apache.org/jira/browse/MESOS-4267
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added helper function to flatten resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp cc8fef9470d779078aa408ed03e747e5a492deaa
> include/mesos/v1/resources.hpp f4892977f8d7b0439db6e9cf7921334f606a496c
> src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a
> src/tests/resources_tests.cpp b42610f1bf8eacfd7bf388d351f8745f1d96f666
> src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc
>
> Diff: https://reviews.apache.org/r/41772/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ResourcesOperationTest.*" --verbose
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Guangya Liu <gy...@gmail.com>.
> On 一月 19, 2016, 10:18 p.m., Joseph Wu wrote:
> > src/common/resources.cpp, line 880
> > <https://reviews.apache.org/r/41772/diff/6/?file=1199181#file1199181line880>
> >
> > It should be fine to just name this `flatten`.
> >
> > You should also consider changing the parameter type from `RevocableInfo::Type` to just `RevocableInfo`. Supplying the whole `RevocableInfo` will save us another tweak to the helpers if we change the revocable protobufs again.
>
> Guangya Liu wrote:
> I think that we cannot rename it to `flatten` as the compiler will not know what does `flatten()` mean.
>
> Klaus Ma wrote:
> Regarding from `RevocableInfo::Type` to `RevocableInfo`, do you mean change `RevocableInfo` from `message` to `enum`? I think it's better to define const in `Resources` instead of changing `RevocableInfo`'s type, e.g. `static const Resource::RevocableInfo::Type Resources::ALLOCATION_SLACK = Resource::RevocableInfo::Type::ALLOCATION_SLACK`.
>
> Regarding `flatten`, there're two options to me:
>
> * Option 1: add `RevocableInfo::Type` to as 3rd parameter in current `flatten` with `None()` as default vaule, and a new helper function to accept `RevocableInfo::Type` only without default value. The sample code is:
>
> ```
> Resources flatten(string role = "*", Option<ReservationInfo> resveration = None(), Option<RevocableInfo::Type> revocable = None()) {
> ...
> if (revocable.isSome()) {
> flatten(revocable.get());
> } else {
> // clear revocable info.
> }
> }
>
> Resources flatten(RevocableInfo::Type revocable) {
> foreach resources {
> resource.mutable_revocable()->set_type(revocable);
> }
> }
> ```
>
> * Option 2: add new `flatten` functions but did not provide default value; `None()` means clear the `RevocableInfo::Type`.
>
> I perfer to Option 1 :). Any comments?
>
> Guangya Liu wrote:
> option 1 will cause all caller who want to flatten allocation slack need to input all of the four parameter, it is really not convenient for the caller.
> Option 2 need to add a new `flatten` with a `requested` but not `option` parameter, I think that maybe 2 is better.
>
> Klaus Ma wrote:
> regarding 1, if want to clear the flags (role, reservation and revocable), call `flatten()`; if want to set `ALLOCATION_SLACK`, call `flatten(Resources::ALLOCATION_SLACK)`.
>
> Guangya Liu wrote:
> Just clarify 1) here, I have some mis-understanding for it.
>
> Option 1) has involved two APIs, one is update current `Resources flatten(string role = "*", Option<ReservationInfo> resveration = None(), , Option<RevocableInfo::Type> revocable = None() )`, the other is add a new `flatten(RevocableInfo::Type revocable)`. The `first flatten` is used to clear `allocation slack` and the `second flatten` is used to set `allocation slack` flag.
>
> Seems 1) is better as it faciliates the caller.
>
> Joseph Wu wrote:
> Given how you're using `flatten`, it would be better to have two calls:
> `Resources flatten(string role = "*", Option<ReservationInfo> resveration = None())`
> and
> `Resources flatten(Option<RevocableInfo> revocable)` // No default so there's no ambiguity.
>
> ---
>
> Note: If you wanted a single `flatten`, you'd need to have `Option<Option<ReservationInfo>>` so that you have the option of setting, un-setting, or not-changing each field.
>
> Guangya Liu wrote:
> I think that adding a new `flatten` without default value might be better and clear.
>
> Guangya Liu wrote:
> But the problem is that even with
>
> `Resources flatten(string role = "*", Option<ReservationInfo> resveration = None())`
> and
> `Resources flatten(Option<RevocableInfo> revocable)`
>
> There are still ambiguity when calling `flatten(role)`, comments?
>
> Guangya Liu wrote:
> In file included from ../../src/tests/fault_tolerance_tests.cpp:56:
> ../../src/tests/mesos.hpp:665:39: error: call to member function 'flatten' is ambiguous
> remaining.find(TASK_RESOURCES.flatten(role));
> ~~~~~~~~~^
> What about still use `flattenSlack`?
>
> Klaus Ma wrote:
> Sorry, just checked this error; it seems we `make` passed but `make check` failed, right?
Yes, the `make tests/check` failed, any idea for this?
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/#review115249
-----------------------------------------------------------
On 一月 23, 2016, 2:59 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41772/
> -----------------------------------------------------------
>
> (Updated 一月 23, 2016, 2:59 a.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
>
>
> Bugs: MESOS-4267
> https://issues.apache.org/jira/browse/MESOS-4267
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added helper function to flatten allocation slack resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp cc8fef9470d779078aa408ed03e747e5a492deaa
> include/mesos/v1/resources.hpp f4892977f8d7b0439db6e9cf7921334f606a496c
> src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a
> src/tests/resources_tests.cpp b42610f1bf8eacfd7bf388d351f8745f1d96f666
> src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc
>
> Diff: https://reviews.apache.org/r/41772/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ResourcesOperationTest.*" --verbose
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Guangya Liu <gy...@gmail.com>.
> On 一月 19, 2016, 10:18 p.m., Joseph Wu wrote:
> > src/common/resources.cpp, line 880
> > <https://reviews.apache.org/r/41772/diff/6/?file=1199181#file1199181line880>
> >
> > It should be fine to just name this `flatten`.
> >
> > You should also consider changing the parameter type from `RevocableInfo::Type` to just `RevocableInfo`. Supplying the whole `RevocableInfo` will save us another tweak to the helpers if we change the revocable protobufs again.
>
> Guangya Liu wrote:
> I think that we cannot rename it to `flatten` as the compiler will not know what does `flatten()` mean.
>
> Klaus Ma wrote:
> Regarding from `RevocableInfo::Type` to `RevocableInfo`, do you mean change `RevocableInfo` from `message` to `enum`? I think it's better to define const in `Resources` instead of changing `RevocableInfo`'s type, e.g. `static const Resource::RevocableInfo::Type Resources::ALLOCATION_SLACK = Resource::RevocableInfo::Type::ALLOCATION_SLACK`.
>
> Regarding `flatten`, there're two options to me:
>
> * Option 1: add `RevocableInfo::Type` to as 3rd parameter in current `flatten` with `None()` as default vaule, and a new helper function to accept `RevocableInfo::Type` only without default value. The sample code is:
>
> ```
> Resources flatten(string role = "*", Option<ReservationInfo> resveration = None(), Option<RevocableInfo::Type> revocable = None()) {
> ...
> if (revocable.isSome()) {
> flatten(revocable.get());
> } else {
> // clear revocable info.
> }
> }
>
> Resources flatten(RevocableInfo::Type revocable) {
> foreach resources {
> resource.mutable_revocable()->set_type(revocable);
> }
> }
> ```
>
> * Option 2: add new `flatten` functions but did not provide default value; `None()` means clear the `RevocableInfo::Type`.
>
> I perfer to Option 1 :). Any comments?
option 1 will cause all caller who want to flatten allocation slack need to input all of the four parameter, it is really not convenient for the caller.
Option 2 need to add a new `flatten` with a `requested` but not `option` parameter, I think that maybe 2 is better.
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/#review115249
-----------------------------------------------------------
On 一月 20, 2016, 3:40 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41772/
> -----------------------------------------------------------
>
> (Updated 一月 20, 2016, 3:40 a.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
>
>
> Bugs: MESOS-4267
> https://issues.apache.org/jira/browse/MESOS-4267
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added helper function to flatten resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp cc8fef9470d779078aa408ed03e747e5a492deaa
> include/mesos/v1/resources.hpp f4892977f8d7b0439db6e9cf7921334f606a496c
> src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a
> src/tests/resources_tests.cpp b42610f1bf8eacfd7bf388d351f8745f1d96f666
> src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc
>
> Diff: https://reviews.apache.org/r/41772/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ResourcesOperationTest.*" --verbose
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Guangya Liu <gy...@gmail.com>.
> On Jan. 19, 2016, 10:18 p.m., Joseph Wu wrote:
> > src/common/resources.cpp, line 880
> > <https://reviews.apache.org/r/41772/diff/6/?file=1199181#file1199181line880>
> >
> > It should be fine to just name this `flatten`.
> >
> > You should also consider changing the parameter type from `RevocableInfo::Type` to just `RevocableInfo`. Supplying the whole `RevocableInfo` will save us another tweak to the helpers if we change the revocable protobufs again.
>
> Guangya Liu wrote:
> I think that we cannot rename it to `flatten` as the compiler will not know what does `flatten()` mean.
>
> Klaus Ma wrote:
> Regarding from `RevocableInfo::Type` to `RevocableInfo`, do you mean change `RevocableInfo` from `message` to `enum`? I think it's better to define const in `Resources` instead of changing `RevocableInfo`'s type, e.g. `static const Resource::RevocableInfo::Type Resources::ALLOCATION_SLACK = Resource::RevocableInfo::Type::ALLOCATION_SLACK`.
>
> Regarding `flatten`, there're two options to me:
>
> * Option 1: add `RevocableInfo::Type` to as 3rd parameter in current `flatten` with `None()` as default vaule, and a new helper function to accept `RevocableInfo::Type` only without default value. The sample code is:
>
> ```
> Resources flatten(string role = "*", Option<ReservationInfo> resveration = None(), Option<RevocableInfo::Type> revocable = None()) {
> ...
> if (revocable.isSome()) {
> flatten(revocable.get());
> } else {
> // clear revocable info.
> }
> }
>
> Resources flatten(RevocableInfo::Type revocable) {
> foreach resources {
> resource.mutable_revocable()->set_type(revocable);
> }
> }
> ```
>
> * Option 2: add new `flatten` functions but did not provide default value; `None()` means clear the `RevocableInfo::Type`.
>
> I perfer to Option 1 :). Any comments?
>
> Guangya Liu wrote:
> option 1 will cause all caller who want to flatten allocation slack need to input all of the four parameter, it is really not convenient for the caller.
> Option 2 need to add a new `flatten` with a `requested` but not `option` parameter, I think that maybe 2 is better.
>
> Klaus Ma wrote:
> regarding 1, if want to clear the flags (role, reservation and revocable), call `flatten()`; if want to set `ALLOCATION_SLACK`, call `flatten(Resources::ALLOCATION_SLACK)`.
>
> Guangya Liu wrote:
> Just clarify 1) here, I have some mis-understanding for it.
>
> Option 1) has involved two APIs, one is update current `Resources flatten(string role = "*", Option<ReservationInfo> resveration = None(), , Option<RevocableInfo::Type> revocable = None() )`, the other is add a new `flatten(RevocableInfo::Type revocable)`. The `first flatten` is used to clear `allocation slack` and the `second flatten` is used to set `allocation slack` flag.
>
> Seems 1) is better as it faciliates the caller.
>
> Joseph Wu wrote:
> Given how you're using `flatten`, it would be better to have two calls:
> `Resources flatten(string role = "*", Option<ReservationInfo> resveration = None())`
> and
> `Resources flatten(Option<RevocableInfo> revocable)` // No default so there's no ambiguity.
>
> ---
>
> Note: If you wanted a single `flatten`, you'd need to have `Option<Option<ReservationInfo>>` so that you have the option of setting, un-setting, or not-changing each field.
>
> Guangya Liu wrote:
> I think that adding a new `flatten` without default value might be better and clear.
>
> Guangya Liu wrote:
> But the problem is that even with
>
> `Resources flatten(string role = "*", Option<ReservationInfo> resveration = None())`
> and
> `Resources flatten(Option<RevocableInfo> revocable)`
>
> There are still ambiguity when calling `flatten(role)`, comments?
>
> Guangya Liu wrote:
> In file included from ../../src/tests/fault_tolerance_tests.cpp:56:
> ../../src/tests/mesos.hpp:665:39: error: call to member function 'flatten' is ambiguous
> remaining.find(TASK_RESOURCES.flatten(role));
> ~~~~~~~~~^
> What about still use `flattenSlack`?
>
> Klaus Ma wrote:
> Sorry, just checked this error; it seems we `make` passed but `make check` failed, right?
>
> Guangya Liu wrote:
> Yes, the `make tests/check` failed, any idea for this?
>
> Joseph Wu wrote:
> It's possible to fix the ambiguity (thanks MPark!), although I'm not sure if we want to...
>
> We can disambiguate by changing one of `Option` 's constructors to:
> ```
> template <
> typename U,
> typename = typename std::enable_if<
> std::is_constructible<T, const U&>::value>::type>
> Option(const U& u);
> ```
Compared with this complex one, I prefer still use another name such as `flattenRevocable` instead, cause we are now introducing another revocable type `EXTRA` in https://reviews.apache.org/r/42742/
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/#review115249
-----------------------------------------------------------
On Jan. 23, 2016, 2:59 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41772/
> -----------------------------------------------------------
>
> (Updated Jan. 23, 2016, 2:59 a.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
>
>
> Bugs: MESOS-4267
> https://issues.apache.org/jira/browse/MESOS-4267
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added helper function to flatten allocation slack resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp cc8fef9470d779078aa408ed03e747e5a492deaa
> include/mesos/v1/resources.hpp f4892977f8d7b0439db6e9cf7921334f606a496c
> src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a
> src/tests/resources_tests.cpp b42610f1bf8eacfd7bf388d351f8745f1d96f666
> src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc
>
> Diff: https://reviews.apache.org/r/41772/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ResourcesOperationTest.*" --verbose
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Klaus Ma <kl...@gmail.com>.
> On Jan. 20, 2016, 6:18 a.m., Joseph Wu wrote:
> > src/common/resources.cpp, line 880
> > <https://reviews.apache.org/r/41772/diff/6/?file=1199181#file1199181line880>
> >
> > It should be fine to just name this `flatten`.
> >
> > You should also consider changing the parameter type from `RevocableInfo::Type` to just `RevocableInfo`. Supplying the whole `RevocableInfo` will save us another tweak to the helpers if we change the revocable protobufs again.
>
> Guangya Liu wrote:
> I think that we cannot rename it to `flatten` as the compiler will not know what does `flatten()` mean.
>
> Klaus Ma wrote:
> Regarding from `RevocableInfo::Type` to `RevocableInfo`, do you mean change `RevocableInfo` from `message` to `enum`? I think it's better to define const in `Resources` instead of changing `RevocableInfo`'s type, e.g. `static const Resource::RevocableInfo::Type Resources::ALLOCATION_SLACK = Resource::RevocableInfo::Type::ALLOCATION_SLACK`.
>
> Regarding `flatten`, there're two options to me:
>
> * Option 1: add `RevocableInfo::Type` to as 3rd parameter in current `flatten` with `None()` as default vaule, and a new helper function to accept `RevocableInfo::Type` only without default value. The sample code is:
>
> ```
> Resources flatten(string role = "*", Option<ReservationInfo> resveration = None(), Option<RevocableInfo::Type> revocable = None()) {
> ...
> if (revocable.isSome()) {
> flatten(revocable.get());
> } else {
> // clear revocable info.
> }
> }
>
> Resources flatten(RevocableInfo::Type revocable) {
> foreach resources {
> resource.mutable_revocable()->set_type(revocable);
> }
> }
> ```
>
> * Option 2: add new `flatten` functions but did not provide default value; `None()` means clear the `RevocableInfo::Type`.
>
> I perfer to Option 1 :). Any comments?
>
> Guangya Liu wrote:
> option 1 will cause all caller who want to flatten allocation slack need to input all of the four parameter, it is really not convenient for the caller.
> Option 2 need to add a new `flatten` with a `requested` but not `option` parameter, I think that maybe 2 is better.
regarding 1, if want to clear the flags (role, reservation and revocable), call `flatten()`; if want to set `ALLOCATION_SLACK`, call `flatten(Resources::ALLOCATION_SLACK)`.
- Klaus
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/#review115249
-----------------------------------------------------------
On Jan. 20, 2016, 11:40 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41772/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2016, 11:40 a.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
>
>
> Bugs: MESOS-4267
> https://issues.apache.org/jira/browse/MESOS-4267
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added helper function to flatten resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp cc8fef9470d779078aa408ed03e747e5a492deaa
> include/mesos/v1/resources.hpp f4892977f8d7b0439db6e9cf7921334f606a496c
> src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a
> src/tests/resources_tests.cpp b42610f1bf8eacfd7bf388d351f8745f1d96f666
> src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc
>
> Diff: https://reviews.apache.org/r/41772/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ResourcesOperationTest.*" --verbose
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Guangya Liu <gy...@gmail.com>.
> On 一月 19, 2016, 10:18 p.m., Joseph Wu wrote:
> > src/common/resources.cpp, line 880
> > <https://reviews.apache.org/r/41772/diff/6/?file=1199181#file1199181line880>
> >
> > It should be fine to just name this `flatten`.
> >
> > You should also consider changing the parameter type from `RevocableInfo::Type` to just `RevocableInfo`. Supplying the whole `RevocableInfo` will save us another tweak to the helpers if we change the revocable protobufs again.
>
> Guangya Liu wrote:
> I think that we cannot rename it to `flatten` as the compiler will not know what does `flatten()` mean.
>
> Klaus Ma wrote:
> Regarding from `RevocableInfo::Type` to `RevocableInfo`, do you mean change `RevocableInfo` from `message` to `enum`? I think it's better to define const in `Resources` instead of changing `RevocableInfo`'s type, e.g. `static const Resource::RevocableInfo::Type Resources::ALLOCATION_SLACK = Resource::RevocableInfo::Type::ALLOCATION_SLACK`.
>
> Regarding `flatten`, there're two options to me:
>
> * Option 1: add `RevocableInfo::Type` to as 3rd parameter in current `flatten` with `None()` as default vaule, and a new helper function to accept `RevocableInfo::Type` only without default value. The sample code is:
>
> ```
> Resources flatten(string role = "*", Option<ReservationInfo> resveration = None(), Option<RevocableInfo::Type> revocable = None()) {
> ...
> if (revocable.isSome()) {
> flatten(revocable.get());
> } else {
> // clear revocable info.
> }
> }
>
> Resources flatten(RevocableInfo::Type revocable) {
> foreach resources {
> resource.mutable_revocable()->set_type(revocable);
> }
> }
> ```
>
> * Option 2: add new `flatten` functions but did not provide default value; `None()` means clear the `RevocableInfo::Type`.
>
> I perfer to Option 1 :). Any comments?
>
> Guangya Liu wrote:
> option 1 will cause all caller who want to flatten allocation slack need to input all of the four parameter, it is really not convenient for the caller.
> Option 2 need to add a new `flatten` with a `requested` but not `option` parameter, I think that maybe 2 is better.
>
> Klaus Ma wrote:
> regarding 1, if want to clear the flags (role, reservation and revocable), call `flatten()`; if want to set `ALLOCATION_SLACK`, call `flatten(Resources::ALLOCATION_SLACK)`.
>
> Guangya Liu wrote:
> Just clarify 1) here, I have some mis-understanding for it.
>
> Option 1) has involved two APIs, one is update current `Resources flatten(string role = "*", Option<ReservationInfo> resveration = None(), , Option<RevocableInfo::Type> revocable = None() )`, the other is add a new `flatten(RevocableInfo::Type revocable)`. The `first flatten` is used to clear `allocation slack` and the `second flatten` is used to set `allocation slack` flag.
>
> Seems 1) is better as it faciliates the caller.
>
> Joseph Wu wrote:
> Given how you're using `flatten`, it would be better to have two calls:
> `Resources flatten(string role = "*", Option<ReservationInfo> resveration = None())`
> and
> `Resources flatten(Option<RevocableInfo> revocable)` // No default so there's no ambiguity.
>
> ---
>
> Note: If you wanted a single `flatten`, you'd need to have `Option<Option<ReservationInfo>>` so that you have the option of setting, un-setting, or not-changing each field.
>
> Guangya Liu wrote:
> I think that adding a new `flatten` without default value might be better and clear.
>
> Guangya Liu wrote:
> But the problem is that even with
>
> `Resources flatten(string role = "*", Option<ReservationInfo> resveration = None())`
> and
> `Resources flatten(Option<RevocableInfo> revocable)`
>
> There are still ambiguity when calling `flatten(role)`, comments?
In file included from ../../src/tests/fault_tolerance_tests.cpp:56:
../../src/tests/mesos.hpp:665:39: error: call to member function 'flatten' is ambiguous
remaining.find(TASK_RESOURCES.flatten(role));
~~~~~~~~~^
What about still use `flattenSlack`?
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/#review115249
-----------------------------------------------------------
On 一月 20, 2016, 3:40 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41772/
> -----------------------------------------------------------
>
> (Updated 一月 20, 2016, 3:40 a.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
>
>
> Bugs: MESOS-4267
> https://issues.apache.org/jira/browse/MESOS-4267
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added helper function to flatten resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp cc8fef9470d779078aa408ed03e747e5a492deaa
> include/mesos/v1/resources.hpp f4892977f8d7b0439db6e9cf7921334f606a496c
> src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a
> src/tests/resources_tests.cpp b42610f1bf8eacfd7bf388d351f8745f1d96f666
> src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc
>
> Diff: https://reviews.apache.org/r/41772/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ResourcesOperationTest.*" --verbose
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Jan. 19, 2016, 2:18 p.m., Joseph Wu wrote:
> > src/common/resources.cpp, line 880
> > <https://reviews.apache.org/r/41772/diff/6/?file=1199181#file1199181line880>
> >
> > It should be fine to just name this `flatten`.
> >
> > You should also consider changing the parameter type from `RevocableInfo::Type` to just `RevocableInfo`. Supplying the whole `RevocableInfo` will save us another tweak to the helpers if we change the revocable protobufs again.
>
> Guangya Liu wrote:
> I think that we cannot rename it to `flatten` as the compiler will not know what does `flatten()` mean.
>
> Klaus Ma wrote:
> Regarding from `RevocableInfo::Type` to `RevocableInfo`, do you mean change `RevocableInfo` from `message` to `enum`? I think it's better to define const in `Resources` instead of changing `RevocableInfo`'s type, e.g. `static const Resource::RevocableInfo::Type Resources::ALLOCATION_SLACK = Resource::RevocableInfo::Type::ALLOCATION_SLACK`.
>
> Regarding `flatten`, there're two options to me:
>
> * Option 1: add `RevocableInfo::Type` to as 3rd parameter in current `flatten` with `None()` as default vaule, and a new helper function to accept `RevocableInfo::Type` only without default value. The sample code is:
>
> ```
> Resources flatten(string role = "*", Option<ReservationInfo> resveration = None(), Option<RevocableInfo::Type> revocable = None()) {
> ...
> if (revocable.isSome()) {
> flatten(revocable.get());
> } else {
> // clear revocable info.
> }
> }
>
> Resources flatten(RevocableInfo::Type revocable) {
> foreach resources {
> resource.mutable_revocable()->set_type(revocable);
> }
> }
> ```
>
> * Option 2: add new `flatten` functions but did not provide default value; `None()` means clear the `RevocableInfo::Type`.
>
> I perfer to Option 1 :). Any comments?
>
> Guangya Liu wrote:
> option 1 will cause all caller who want to flatten allocation slack need to input all of the four parameter, it is really not convenient for the caller.
> Option 2 need to add a new `flatten` with a `requested` but not `option` parameter, I think that maybe 2 is better.
>
> Klaus Ma wrote:
> regarding 1, if want to clear the flags (role, reservation and revocable), call `flatten()`; if want to set `ALLOCATION_SLACK`, call `flatten(Resources::ALLOCATION_SLACK)`.
>
> Guangya Liu wrote:
> Just clarify 1) here, I have some mis-understanding for it.
>
> Option 1) has involved two APIs, one is update current `Resources flatten(string role = "*", Option<ReservationInfo> resveration = None(), , Option<RevocableInfo::Type> revocable = None() )`, the other is add a new `flatten(RevocableInfo::Type revocable)`. The `first flatten` is used to clear `allocation slack` and the `second flatten` is used to set `allocation slack` flag.
>
> Seems 1) is better as it faciliates the caller.
>
> Joseph Wu wrote:
> Given how you're using `flatten`, it would be better to have two calls:
> `Resources flatten(string role = "*", Option<ReservationInfo> resveration = None())`
> and
> `Resources flatten(Option<RevocableInfo> revocable)` // No default so there's no ambiguity.
>
> ---
>
> Note: If you wanted a single `flatten`, you'd need to have `Option<Option<ReservationInfo>>` so that you have the option of setting, un-setting, or not-changing each field.
>
> Guangya Liu wrote:
> I think that adding a new `flatten` without default value might be better and clear.
>
> Guangya Liu wrote:
> But the problem is that even with
>
> `Resources flatten(string role = "*", Option<ReservationInfo> resveration = None())`
> and
> `Resources flatten(Option<RevocableInfo> revocable)`
>
> There are still ambiguity when calling `flatten(role)`, comments?
>
> Guangya Liu wrote:
> In file included from ../../src/tests/fault_tolerance_tests.cpp:56:
> ../../src/tests/mesos.hpp:665:39: error: call to member function 'flatten' is ambiguous
> remaining.find(TASK_RESOURCES.flatten(role));
> ~~~~~~~~~^
> What about still use `flattenSlack`?
>
> Klaus Ma wrote:
> Sorry, just checked this error; it seems we `make` passed but `make check` failed, right?
>
> Guangya Liu wrote:
> Yes, the `make tests/check` failed, any idea for this?
It's possible to fix the ambiguity (thanks MPark!), although I'm not sure if we want to...
We can disambiguate by changing one of `Option` 's constructors to:
```
template <
typename U,
typename = typename std::enable_if<
std::is_constructible<T, const U&>::value>::type>
Option(const U& u);
```
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/#review115249
-----------------------------------------------------------
On Jan. 22, 2016, 6:59 p.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41772/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2016, 6:59 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
>
>
> Bugs: MESOS-4267
> https://issues.apache.org/jira/browse/MESOS-4267
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added helper function to flatten allocation slack resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp cc8fef9470d779078aa408ed03e747e5a492deaa
> include/mesos/v1/resources.hpp f4892977f8d7b0439db6e9cf7921334f606a496c
> src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a
> src/tests/resources_tests.cpp b42610f1bf8eacfd7bf388d351f8745f1d96f666
> src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc
>
> Diff: https://reviews.apache.org/r/41772/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ResourcesOperationTest.*" --verbose
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Guangya Liu <gy...@gmail.com>.
> On 一月 19, 2016, 10:18 p.m., Joseph Wu wrote:
> > src/common/resources.cpp, lines 874-877
> > <https://reviews.apache.org/r/41772/diff/6/?file=1199181#file1199181line874>
> >
> > This is pretty much a copy of `Resources::reserved` now. You can remove it.
>
> Guangya Liu wrote:
> The reason that I keep this is because the current `Resources::reserved` returns a hashmap but what I need here is a flatten string for resources in different roles. e.g. cpus(r1):100;cpus(r2):200
>
> I updated the comments here to clarify why this is needed.
>
> Klaus Ma wrote:
> I think it's more general to add a `Resources` constructor to accept hashmap, so it'll be `Resources(res.reserved()).flatten()`.
>
> Joseph Wu wrote:
> It doesn't look like `reserved()` is used very heavily. And the places that do use it could easily use a normal `Resources` object. I'd recommend changing `reserved()` or just using `reserved("*")`.
I created a new patch here https://reviews.apache.org/r/42590/ , can you help review? I think that the `reserved()` API can be removed.
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/#review115249
-----------------------------------------------------------
On 一月 20, 2016, 3:40 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41772/
> -----------------------------------------------------------
>
> (Updated 一月 20, 2016, 3:40 a.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
>
>
> Bugs: MESOS-4267
> https://issues.apache.org/jira/browse/MESOS-4267
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added helper function to flatten resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp cc8fef9470d779078aa408ed03e747e5a492deaa
> include/mesos/v1/resources.hpp f4892977f8d7b0439db6e9cf7921334f606a496c
> src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a
> src/tests/resources_tests.cpp b42610f1bf8eacfd7bf388d351f8745f1d96f666
> src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc
>
> Diff: https://reviews.apache.org/r/41772/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ResourcesOperationTest.*" --verbose
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Guangya Liu <gy...@gmail.com>.
> On 一月 19, 2016, 10:18 p.m., Joseph Wu wrote:
> > src/common/resources.cpp, line 880
> > <https://reviews.apache.org/r/41772/diff/6/?file=1199181#file1199181line880>
> >
> > It should be fine to just name this `flatten`.
> >
> > You should also consider changing the parameter type from `RevocableInfo::Type` to just `RevocableInfo`. Supplying the whole `RevocableInfo` will save us another tweak to the helpers if we change the revocable protobufs again.
>
> Guangya Liu wrote:
> I think that we cannot rename it to `flatten` as the compiler will not know what does `flatten()` mean.
>
> Klaus Ma wrote:
> Regarding from `RevocableInfo::Type` to `RevocableInfo`, do you mean change `RevocableInfo` from `message` to `enum`? I think it's better to define const in `Resources` instead of changing `RevocableInfo`'s type, e.g. `static const Resource::RevocableInfo::Type Resources::ALLOCATION_SLACK = Resource::RevocableInfo::Type::ALLOCATION_SLACK`.
>
> Regarding `flatten`, there're two options to me:
>
> * Option 1: add `RevocableInfo::Type` to as 3rd parameter in current `flatten` with `None()` as default vaule, and a new helper function to accept `RevocableInfo::Type` only without default value. The sample code is:
>
> ```
> Resources flatten(string role = "*", Option<ReservationInfo> resveration = None(), Option<RevocableInfo::Type> revocable = None()) {
> ...
> if (revocable.isSome()) {
> flatten(revocable.get());
> } else {
> // clear revocable info.
> }
> }
>
> Resources flatten(RevocableInfo::Type revocable) {
> foreach resources {
> resource.mutable_revocable()->set_type(revocable);
> }
> }
> ```
>
> * Option 2: add new `flatten` functions but did not provide default value; `None()` means clear the `RevocableInfo::Type`.
>
> I perfer to Option 1 :). Any comments?
>
> Guangya Liu wrote:
> option 1 will cause all caller who want to flatten allocation slack need to input all of the four parameter, it is really not convenient for the caller.
> Option 2 need to add a new `flatten` with a `requested` but not `option` parameter, I think that maybe 2 is better.
>
> Klaus Ma wrote:
> regarding 1, if want to clear the flags (role, reservation and revocable), call `flatten()`; if want to set `ALLOCATION_SLACK`, call `flatten(Resources::ALLOCATION_SLACK)`.
>
> Guangya Liu wrote:
> Just clarify 1) here, I have some mis-understanding for it.
>
> Option 1) has involved two APIs, one is update current `Resources flatten(string role = "*", Option<ReservationInfo> resveration = None(), , Option<RevocableInfo::Type> revocable = None() )`, the other is add a new `flatten(RevocableInfo::Type revocable)`. The `first flatten` is used to clear `allocation slack` and the `second flatten` is used to set `allocation slack` flag.
>
> Seems 1) is better as it faciliates the caller.
>
> Joseph Wu wrote:
> Given how you're using `flatten`, it would be better to have two calls:
> `Resources flatten(string role = "*", Option<ReservationInfo> resveration = None())`
> and
> `Resources flatten(Option<RevocableInfo> revocable)` // No default so there's no ambiguity.
>
> ---
>
> Note: If you wanted a single `flatten`, you'd need to have `Option<Option<ReservationInfo>>` so that you have the option of setting, un-setting, or not-changing each field.
>
> Guangya Liu wrote:
> I think that adding a new `flatten` without default value might be better and clear.
But the problem is that even with
`Resources flatten(string role = "*", Option<ReservationInfo> resveration = None())`
and
`Resources flatten(Option<RevocableInfo> revocable)`
There are still ambiguity when calling `flatten(role)`, comments?
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/#review115249
-----------------------------------------------------------
On 一月 20, 2016, 3:40 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41772/
> -----------------------------------------------------------
>
> (Updated 一月 20, 2016, 3:40 a.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
>
>
> Bugs: MESOS-4267
> https://issues.apache.org/jira/browse/MESOS-4267
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added helper function to flatten resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp cc8fef9470d779078aa408ed03e747e5a492deaa
> include/mesos/v1/resources.hpp f4892977f8d7b0439db6e9cf7921334f606a496c
> src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a
> src/tests/resources_tests.cpp b42610f1bf8eacfd7bf388d351f8745f1d96f666
> src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc
>
> Diff: https://reviews.apache.org/r/41772/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ResourcesOperationTest.*" --verbose
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Guangya Liu <gy...@gmail.com>.
> On 一月 19, 2016, 10:18 p.m., Joseph Wu wrote:
> > src/common/resources.cpp, line 880
> > <https://reviews.apache.org/r/41772/diff/6/?file=1199181#file1199181line880>
> >
> > It should be fine to just name this `flatten`.
> >
> > You should also consider changing the parameter type from `RevocableInfo::Type` to just `RevocableInfo`. Supplying the whole `RevocableInfo` will save us another tweak to the helpers if we change the revocable protobufs again.
>
> Guangya Liu wrote:
> I think that we cannot rename it to `flatten` as the compiler will not know what does `flatten()` mean.
>
> Klaus Ma wrote:
> Regarding from `RevocableInfo::Type` to `RevocableInfo`, do you mean change `RevocableInfo` from `message` to `enum`? I think it's better to define const in `Resources` instead of changing `RevocableInfo`'s type, e.g. `static const Resource::RevocableInfo::Type Resources::ALLOCATION_SLACK = Resource::RevocableInfo::Type::ALLOCATION_SLACK`.
>
> Regarding `flatten`, there're two options to me:
>
> * Option 1: add `RevocableInfo::Type` to as 3rd parameter in current `flatten` with `None()` as default vaule, and a new helper function to accept `RevocableInfo::Type` only without default value. The sample code is:
>
> ```
> Resources flatten(string role = "*", Option<ReservationInfo> resveration = None(), Option<RevocableInfo::Type> revocable = None()) {
> ...
> if (revocable.isSome()) {
> flatten(revocable.get());
> } else {
> // clear revocable info.
> }
> }
>
> Resources flatten(RevocableInfo::Type revocable) {
> foreach resources {
> resource.mutable_revocable()->set_type(revocable);
> }
> }
> ```
>
> * Option 2: add new `flatten` functions but did not provide default value; `None()` means clear the `RevocableInfo::Type`.
>
> I perfer to Option 1 :). Any comments?
>
> Guangya Liu wrote:
> option 1 will cause all caller who want to flatten allocation slack need to input all of the four parameter, it is really not convenient for the caller.
> Option 2 need to add a new `flatten` with a `requested` but not `option` parameter, I think that maybe 2 is better.
>
> Klaus Ma wrote:
> regarding 1, if want to clear the flags (role, reservation and revocable), call `flatten()`; if want to set `ALLOCATION_SLACK`, call `flatten(Resources::ALLOCATION_SLACK)`.
>
> Guangya Liu wrote:
> Just clarify 1) here, I have some mis-understanding for it.
>
> Option 1) has involved two APIs, one is update current `Resources flatten(string role = "*", Option<ReservationInfo> resveration = None(), , Option<RevocableInfo::Type> revocable = None() )`, the other is add a new `flatten(RevocableInfo::Type revocable)`. The `first flatten` is used to clear `allocation slack` and the `second flatten` is used to set `allocation slack` flag.
>
> Seems 1) is better as it faciliates the caller.
>
> Joseph Wu wrote:
> Given how you're using `flatten`, it would be better to have two calls:
> `Resources flatten(string role = "*", Option<ReservationInfo> resveration = None())`
> and
> `Resources flatten(Option<RevocableInfo> revocable)` // No default so there's no ambiguity.
>
> ---
>
> Note: If you wanted a single `flatten`, you'd need to have `Option<Option<ReservationInfo>>` so that you have the option of setting, un-setting, or not-changing each field.
I think that adding a new `flatten` without default value might be better and clear.
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/#review115249
-----------------------------------------------------------
On 一月 20, 2016, 3:40 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41772/
> -----------------------------------------------------------
>
> (Updated 一月 20, 2016, 3:40 a.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
>
>
> Bugs: MESOS-4267
> https://issues.apache.org/jira/browse/MESOS-4267
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added helper function to flatten resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp cc8fef9470d779078aa408ed03e747e5a492deaa
> include/mesos/v1/resources.hpp f4892977f8d7b0439db6e9cf7921334f606a496c
> src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a
> src/tests/resources_tests.cpp b42610f1bf8eacfd7bf388d351f8745f1d96f666
> src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc
>
> Diff: https://reviews.apache.org/r/41772/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ResourcesOperationTest.*" --verbose
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Klaus Ma <kl...@gmail.com>.
> On Jan. 20, 2016, 6:18 a.m., Joseph Wu wrote:
> > src/common/resources.cpp, line 880
> > <https://reviews.apache.org/r/41772/diff/6/?file=1199181#file1199181line880>
> >
> > It should be fine to just name this `flatten`.
> >
> > You should also consider changing the parameter type from `RevocableInfo::Type` to just `RevocableInfo`. Supplying the whole `RevocableInfo` will save us another tweak to the helpers if we change the revocable protobufs again.
>
> Guangya Liu wrote:
> I think that we cannot rename it to `flatten` as the compiler will not know what does `flatten()` mean.
>
> Klaus Ma wrote:
> Regarding from `RevocableInfo::Type` to `RevocableInfo`, do you mean change `RevocableInfo` from `message` to `enum`? I think it's better to define const in `Resources` instead of changing `RevocableInfo`'s type, e.g. `static const Resource::RevocableInfo::Type Resources::ALLOCATION_SLACK = Resource::RevocableInfo::Type::ALLOCATION_SLACK`.
>
> Regarding `flatten`, there're two options to me:
>
> * Option 1: add `RevocableInfo::Type` to as 3rd parameter in current `flatten` with `None()` as default vaule, and a new helper function to accept `RevocableInfo::Type` only without default value. The sample code is:
>
> ```
> Resources flatten(string role = "*", Option<ReservationInfo> resveration = None(), Option<RevocableInfo::Type> revocable = None()) {
> ...
> if (revocable.isSome()) {
> flatten(revocable.get());
> } else {
> // clear revocable info.
> }
> }
>
> Resources flatten(RevocableInfo::Type revocable) {
> foreach resources {
> resource.mutable_revocable()->set_type(revocable);
> }
> }
> ```
>
> * Option 2: add new `flatten` functions but did not provide default value; `None()` means clear the `RevocableInfo::Type`.
>
> I perfer to Option 1 :). Any comments?
>
> Guangya Liu wrote:
> option 1 will cause all caller who want to flatten allocation slack need to input all of the four parameter, it is really not convenient for the caller.
> Option 2 need to add a new `flatten` with a `requested` but not `option` parameter, I think that maybe 2 is better.
>
> Klaus Ma wrote:
> regarding 1, if want to clear the flags (role, reservation and revocable), call `flatten()`; if want to set `ALLOCATION_SLACK`, call `flatten(Resources::ALLOCATION_SLACK)`.
>
> Guangya Liu wrote:
> Just clarify 1) here, I have some mis-understanding for it.
>
> Option 1) has involved two APIs, one is update current `Resources flatten(string role = "*", Option<ReservationInfo> resveration = None(), , Option<RevocableInfo::Type> revocable = None() )`, the other is add a new `flatten(RevocableInfo::Type revocable)`. The `first flatten` is used to clear `allocation slack` and the `second flatten` is used to set `allocation slack` flag.
>
> Seems 1) is better as it faciliates the caller.
>
> Joseph Wu wrote:
> Given how you're using `flatten`, it would be better to have two calls:
> `Resources flatten(string role = "*", Option<ReservationInfo> resveration = None())`
> and
> `Resources flatten(Option<RevocableInfo> revocable)` // No default so there's no ambiguity.
>
> ---
>
> Note: If you wanted a single `flatten`, you'd need to have `Option<Option<ReservationInfo>>` so that you have the option of setting, un-setting, or not-changing each field.
>
> Guangya Liu wrote:
> I think that adding a new `flatten` without default value might be better and clear.
>
> Guangya Liu wrote:
> But the problem is that even with
>
> `Resources flatten(string role = "*", Option<ReservationInfo> resveration = None())`
> and
> `Resources flatten(Option<RevocableInfo> revocable)`
>
> There are still ambiguity when calling `flatten(role)`, comments?
>
> Guangya Liu wrote:
> In file included from ../../src/tests/fault_tolerance_tests.cpp:56:
> ../../src/tests/mesos.hpp:665:39: error: call to member function 'flatten' is ambiguous
> remaining.find(TASK_RESOURCES.flatten(role));
> ~~~~~~~~~^
> What about still use `flattenSlack`?
Sorry, just checked this error; it seems we `make` passed but `make check` failed, right?
- Klaus
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/#review115249
-----------------------------------------------------------
On Jan. 23, 2016, 10:59 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41772/
> -----------------------------------------------------------
>
> (Updated Jan. 23, 2016, 10:59 a.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
>
>
> Bugs: MESOS-4267
> https://issues.apache.org/jira/browse/MESOS-4267
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added helper function to flatten allocation slack resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp cc8fef9470d779078aa408ed03e747e5a492deaa
> include/mesos/v1/resources.hpp f4892977f8d7b0439db6e9cf7921334f606a496c
> src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a
> src/tests/resources_tests.cpp b42610f1bf8eacfd7bf388d351f8745f1d96f666
> src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc
>
> Diff: https://reviews.apache.org/r/41772/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ResourcesOperationTest.*" --verbose
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Guangya Liu <gy...@gmail.com>.
> On 一月 19, 2016, 10:18 p.m., Joseph Wu wrote:
> > src/common/resources.cpp, lines 874-877
> > <https://reviews.apache.org/r/41772/diff/6/?file=1199181#file1199181line874>
> >
> > This is pretty much a copy of `Resources::reserved` now. You can remove it.
The reason that I keep this is because the current `Resources::reserved` returns a hashmap but what I need here is a flatten string for resources in different roles. e.g. cpus(r1):100;cpus(r2):200
I updated the comments here to clarify why this is needed.
> On 一月 19, 2016, 10:18 p.m., Joseph Wu wrote:
> > src/common/resources.cpp, line 880
> > <https://reviews.apache.org/r/41772/diff/6/?file=1199181#file1199181line880>
> >
> > It should be fine to just name this `flatten`.
> >
> > You should also consider changing the parameter type from `RevocableInfo::Type` to just `RevocableInfo`. Supplying the whole `RevocableInfo` will save us another tweak to the helpers if we change the revocable protobufs again.
I think that we cannot rename it to `flatten` as the compiler will not know what does `flatten()` mean.
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/#review115249
-----------------------------------------------------------
On 一月 16, 2016, 4:23 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41772/
> -----------------------------------------------------------
>
> (Updated 一月 16, 2016, 4:23 a.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
>
>
> Bugs: MESOS-4267
> https://issues.apache.org/jira/browse/MESOS-4267
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added helper function to flatten resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp cc8fef9470d779078aa408ed03e747e5a492deaa
> include/mesos/v1/resources.hpp f4892977f8d7b0439db6e9cf7921334f606a496c
> src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a
> src/tests/resources_tests.cpp b42610f1bf8eacfd7bf388d351f8745f1d96f666
> src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc
>
> Diff: https://reviews.apache.org/r/41772/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ResourcesOperationTest.*" --verbose
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Klaus Ma <kl...@gmail.com>.
> On Jan. 20, 2016, 6:18 a.m., Joseph Wu wrote:
> > src/common/resources.cpp, line 880
> > <https://reviews.apache.org/r/41772/diff/6/?file=1199181#file1199181line880>
> >
> > It should be fine to just name this `flatten`.
> >
> > You should also consider changing the parameter type from `RevocableInfo::Type` to just `RevocableInfo`. Supplying the whole `RevocableInfo` will save us another tweak to the helpers if we change the revocable protobufs again.
>
> Guangya Liu wrote:
> I think that we cannot rename it to `flatten` as the compiler will not know what does `flatten()` mean.
>
> Klaus Ma wrote:
> Regarding from `RevocableInfo::Type` to `RevocableInfo`, do you mean change `RevocableInfo` from `message` to `enum`? I think it's better to define const in `Resources` instead of changing `RevocableInfo`'s type, e.g. `static const Resource::RevocableInfo::Type Resources::ALLOCATION_SLACK = Resource::RevocableInfo::Type::ALLOCATION_SLACK`.
>
> Regarding `flatten`, there're two options to me:
>
> * Option 1: add `RevocableInfo::Type` to as 3rd parameter in current `flatten` with `None()` as default vaule, and a new helper function to accept `RevocableInfo::Type` only without default value. The sample code is:
>
> ```
> Resources flatten(string role = "*", Option<ReservationInfo> resveration = None(), Option<RevocableInfo::Type> revocable = None()) {
> ...
> if (revocable.isSome()) {
> flatten(revocable.get());
> } else {
> // clear revocable info.
> }
> }
>
> Resources flatten(RevocableInfo::Type revocable) {
> foreach resources {
> resource.mutable_revocable()->set_type(revocable);
> }
> }
> ```
>
> * Option 2: add new `flatten` functions but did not provide default value; `None()` means clear the `RevocableInfo::Type`.
>
> I perfer to Option 1 :). Any comments?
>
> Guangya Liu wrote:
> option 1 will cause all caller who want to flatten allocation slack need to input all of the four parameter, it is really not convenient for the caller.
> Option 2 need to add a new `flatten` with a `requested` but not `option` parameter, I think that maybe 2 is better.
>
> Klaus Ma wrote:
> regarding 1, if want to clear the flags (role, reservation and revocable), call `flatten()`; if want to set `ALLOCATION_SLACK`, call `flatten(Resources::ALLOCATION_SLACK)`.
>
> Guangya Liu wrote:
> Just clarify 1) here, I have some mis-understanding for it.
>
> Option 1) has involved two APIs, one is update current `Resources flatten(string role = "*", Option<ReservationInfo> resveration = None(), , Option<RevocableInfo::Type> revocable = None() )`, the other is add a new `flatten(RevocableInfo::Type revocable)`. The `first flatten` is used to clear `allocation slack` and the `second flatten` is used to set `allocation slack` flag.
>
> Seems 1) is better as it faciliates the caller.
>
> Joseph Wu wrote:
> Given how you're using `flatten`, it would be better to have two calls:
> `Resources flatten(string role = "*", Option<ReservationInfo> resveration = None())`
> and
> `Resources flatten(Option<RevocableInfo> revocable)` // No default so there's no ambiguity.
>
> ---
>
> Note: If you wanted a single `flatten`, you'd need to have `Option<Option<ReservationInfo>>` so that you have the option of setting, un-setting, or not-changing each field.
>
> Guangya Liu wrote:
> I think that adding a new `flatten` without default value might be better and clear.
>
> Guangya Liu wrote:
> But the problem is that even with
>
> `Resources flatten(string role = "*", Option<ReservationInfo> resveration = None())`
> and
> `Resources flatten(Option<RevocableInfo> revocable)`
>
> There are still ambiguity when calling `flatten(role)`, comments?
>
> Guangya Liu wrote:
> In file included from ../../src/tests/fault_tolerance_tests.cpp:56:
> ../../src/tests/mesos.hpp:665:39: error: call to member function 'flatten' is ambiguous
> remaining.find(TASK_RESOURCES.flatten(role));
> ~~~~~~~~~^
> What about still use `flattenSlack`?
>
> Klaus Ma wrote:
> Sorry, just checked this error; it seems we `make` passed but `make check` failed, right?
>
> Guangya Liu wrote:
> Yes, the `make tests/check` failed, any idea for this?
>
> Joseph Wu wrote:
> It's possible to fix the ambiguity (thanks MPark!), although I'm not sure if we want to...
>
> We can disambiguate by changing one of `Option` 's constructors to:
> ```
> template <
> typename U,
> typename = typename std::enable_if<
> std::is_constructible<T, const U&>::value>::type>
> Option(const U& u);
> ```
>
> Guangya Liu wrote:
> Compared with this complex one, I prefer still use another name such as `flattenRevocable` instead, cause we are now introducing another revocable type `EXTRA` in https://reviews.apache.org/r/42742/
Sorry for the late :). @Guangya, can you help to rebase this patch? I'd like to have a try for this compile error.
- Klaus
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/#review115249
-----------------------------------------------------------
On Jan. 23, 2016, 10:59 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41772/
> -----------------------------------------------------------
>
> (Updated Jan. 23, 2016, 10:59 a.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
>
>
> Bugs: MESOS-4267
> https://issues.apache.org/jira/browse/MESOS-4267
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added helper function to flatten allocation slack resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp cc8fef9470d779078aa408ed03e747e5a492deaa
> include/mesos/v1/resources.hpp f4892977f8d7b0439db6e9cf7921334f606a496c
> src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a
> src/tests/resources_tests.cpp b42610f1bf8eacfd7bf388d351f8745f1d96f666
> src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc
>
> Diff: https://reviews.apache.org/r/41772/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ResourcesOperationTest.*" --verbose
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Klaus Ma <kl...@gmail.com>.
> On Jan. 20, 2016, 6:18 a.m., Joseph Wu wrote:
> > src/common/resources.cpp, lines 874-877
> > <https://reviews.apache.org/r/41772/diff/6/?file=1199181#file1199181line874>
> >
> > This is pretty much a copy of `Resources::reserved` now. You can remove it.
>
> Guangya Liu wrote:
> The reason that I keep this is because the current `Resources::reserved` returns a hashmap but what I need here is a flatten string for resources in different roles. e.g. cpus(r1):100;cpus(r2):200
>
> I updated the comments here to clarify why this is needed.
I think it's more general to add a `Resources` constructor to accept hashmap, so it'll be `Resources(res.reserved()).flatten()`.
- Klaus
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/#review115249
-----------------------------------------------------------
On Jan. 20, 2016, 11:40 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41772/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2016, 11:40 a.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
>
>
> Bugs: MESOS-4267
> https://issues.apache.org/jira/browse/MESOS-4267
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added helper function to flatten resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp cc8fef9470d779078aa408ed03e747e5a492deaa
> include/mesos/v1/resources.hpp f4892977f8d7b0439db6e9cf7921334f606a496c
> src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a
> src/tests/resources_tests.cpp b42610f1bf8eacfd7bf388d351f8745f1d96f666
> src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc
>
> Diff: https://reviews.apache.org/r/41772/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ResourcesOperationTest.*" --verbose
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Guangya Liu <gy...@gmail.com>.
> On 一月 19, 2016, 10:18 p.m., Joseph Wu wrote:
> > src/common/resources.cpp, line 880
> > <https://reviews.apache.org/r/41772/diff/6/?file=1199181#file1199181line880>
> >
> > It should be fine to just name this `flatten`.
> >
> > You should also consider changing the parameter type from `RevocableInfo::Type` to just `RevocableInfo`. Supplying the whole `RevocableInfo` will save us another tweak to the helpers if we change the revocable protobufs again.
>
> Guangya Liu wrote:
> I think that we cannot rename it to `flatten` as the compiler will not know what does `flatten()` mean.
>
> Klaus Ma wrote:
> Regarding from `RevocableInfo::Type` to `RevocableInfo`, do you mean change `RevocableInfo` from `message` to `enum`? I think it's better to define const in `Resources` instead of changing `RevocableInfo`'s type, e.g. `static const Resource::RevocableInfo::Type Resources::ALLOCATION_SLACK = Resource::RevocableInfo::Type::ALLOCATION_SLACK`.
>
> Regarding `flatten`, there're two options to me:
>
> * Option 1: add `RevocableInfo::Type` to as 3rd parameter in current `flatten` with `None()` as default vaule, and a new helper function to accept `RevocableInfo::Type` only without default value. The sample code is:
>
> ```
> Resources flatten(string role = "*", Option<ReservationInfo> resveration = None(), Option<RevocableInfo::Type> revocable = None()) {
> ...
> if (revocable.isSome()) {
> flatten(revocable.get());
> } else {
> // clear revocable info.
> }
> }
>
> Resources flatten(RevocableInfo::Type revocable) {
> foreach resources {
> resource.mutable_revocable()->set_type(revocable);
> }
> }
> ```
>
> * Option 2: add new `flatten` functions but did not provide default value; `None()` means clear the `RevocableInfo::Type`.
>
> I perfer to Option 1 :). Any comments?
>
> Guangya Liu wrote:
> option 1 will cause all caller who want to flatten allocation slack need to input all of the four parameter, it is really not convenient for the caller.
> Option 2 need to add a new `flatten` with a `requested` but not `option` parameter, I think that maybe 2 is better.
>
> Klaus Ma wrote:
> regarding 1, if want to clear the flags (role, reservation and revocable), call `flatten()`; if want to set `ALLOCATION_SLACK`, call `flatten(Resources::ALLOCATION_SLACK)`.
Just clarify 1) here, I have some mis-understanding for it.
Option 1) has involved two APIs, one is update current `Resources flatten(string role = "*", Option<ReservationInfo> resveration = None(), , Option<RevocableInfo::Type> revocable = None() )`, the other is add a new `flatten(RevocableInfo::Type revocable)`. The `first flatten` is used to clear `allocation slack` and the `second flatten` is used to set `allocation slack` flag.
Seems 1) is better as it faciliates the caller.
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/#review115249
-----------------------------------------------------------
On 一月 20, 2016, 3:40 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41772/
> -----------------------------------------------------------
>
> (Updated 一月 20, 2016, 3:40 a.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
>
>
> Bugs: MESOS-4267
> https://issues.apache.org/jira/browse/MESOS-4267
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added helper function to flatten resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp cc8fef9470d779078aa408ed03e747e5a492deaa
> include/mesos/v1/resources.hpp f4892977f8d7b0439db6e9cf7921334f606a496c
> src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a
> src/tests/resources_tests.cpp b42610f1bf8eacfd7bf388d351f8745f1d96f666
> src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc
>
> Diff: https://reviews.apache.org/r/41772/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ResourcesOperationTest.*" --verbose
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Jan. 19, 2016, 2:18 p.m., Joseph Wu wrote:
> > src/common/resources.cpp, lines 874-877
> > <https://reviews.apache.org/r/41772/diff/6/?file=1199181#file1199181line874>
> >
> > This is pretty much a copy of `Resources::reserved` now. You can remove it.
>
> Guangya Liu wrote:
> The reason that I keep this is because the current `Resources::reserved` returns a hashmap but what I need here is a flatten string for resources in different roles. e.g. cpus(r1):100;cpus(r2):200
>
> I updated the comments here to clarify why this is needed.
>
> Klaus Ma wrote:
> I think it's more general to add a `Resources` constructor to accept hashmap, so it'll be `Resources(res.reserved()).flatten()`.
It doesn't look like `reserved()` is used very heavily. And the places that do use it could easily use a normal `Resources` object. I'd recommend changing `reserved()` or just using `reserved("*")`.
> On Jan. 19, 2016, 2:18 p.m., Joseph Wu wrote:
> > src/common/resources.cpp, line 880
> > <https://reviews.apache.org/r/41772/diff/6/?file=1199181#file1199181line880>
> >
> > It should be fine to just name this `flatten`.
> >
> > You should also consider changing the parameter type from `RevocableInfo::Type` to just `RevocableInfo`. Supplying the whole `RevocableInfo` will save us another tweak to the helpers if we change the revocable protobufs again.
>
> Guangya Liu wrote:
> I think that we cannot rename it to `flatten` as the compiler will not know what does `flatten()` mean.
>
> Klaus Ma wrote:
> Regarding from `RevocableInfo::Type` to `RevocableInfo`, do you mean change `RevocableInfo` from `message` to `enum`? I think it's better to define const in `Resources` instead of changing `RevocableInfo`'s type, e.g. `static const Resource::RevocableInfo::Type Resources::ALLOCATION_SLACK = Resource::RevocableInfo::Type::ALLOCATION_SLACK`.
>
> Regarding `flatten`, there're two options to me:
>
> * Option 1: add `RevocableInfo::Type` to as 3rd parameter in current `flatten` with `None()` as default vaule, and a new helper function to accept `RevocableInfo::Type` only without default value. The sample code is:
>
> ```
> Resources flatten(string role = "*", Option<ReservationInfo> resveration = None(), Option<RevocableInfo::Type> revocable = None()) {
> ...
> if (revocable.isSome()) {
> flatten(revocable.get());
> } else {
> // clear revocable info.
> }
> }
>
> Resources flatten(RevocableInfo::Type revocable) {
> foreach resources {
> resource.mutable_revocable()->set_type(revocable);
> }
> }
> ```
>
> * Option 2: add new `flatten` functions but did not provide default value; `None()` means clear the `RevocableInfo::Type`.
>
> I perfer to Option 1 :). Any comments?
>
> Guangya Liu wrote:
> option 1 will cause all caller who want to flatten allocation slack need to input all of the four parameter, it is really not convenient for the caller.
> Option 2 need to add a new `flatten` with a `requested` but not `option` parameter, I think that maybe 2 is better.
>
> Klaus Ma wrote:
> regarding 1, if want to clear the flags (role, reservation and revocable), call `flatten()`; if want to set `ALLOCATION_SLACK`, call `flatten(Resources::ALLOCATION_SLACK)`.
>
> Guangya Liu wrote:
> Just clarify 1) here, I have some mis-understanding for it.
>
> Option 1) has involved two APIs, one is update current `Resources flatten(string role = "*", Option<ReservationInfo> resveration = None(), , Option<RevocableInfo::Type> revocable = None() )`, the other is add a new `flatten(RevocableInfo::Type revocable)`. The `first flatten` is used to clear `allocation slack` and the `second flatten` is used to set `allocation slack` flag.
>
> Seems 1) is better as it faciliates the caller.
Given how you're using `flatten`, it would be better to have two calls:
`Resources flatten(string role = "*", Option<ReservationInfo> resveration = None())`
and
`Resources flatten(Option<RevocableInfo> revocable)` // No default so there's no ambiguity.
---
Note: If you wanted a single `flatten`, you'd need to have `Option<Option<ReservationInfo>>` so that you have the option of setting, un-setting, or not-changing each field.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/#review115249
-----------------------------------------------------------
On Jan. 19, 2016, 7:40 p.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41772/
> -----------------------------------------------------------
>
> (Updated Jan. 19, 2016, 7:40 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
>
>
> Bugs: MESOS-4267
> https://issues.apache.org/jira/browse/MESOS-4267
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added helper function to flatten resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp cc8fef9470d779078aa408ed03e747e5a492deaa
> include/mesos/v1/resources.hpp f4892977f8d7b0439db6e9cf7921334f606a496c
> src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a
> src/tests/resources_tests.cpp b42610f1bf8eacfd7bf388d351f8745f1d96f666
> src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc
>
> Diff: https://reviews.apache.org/r/41772/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ResourcesOperationTest.*" --verbose
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/#review115249
-----------------------------------------------------------
include/mesos/resources.hpp (lines 277 - 278)
<https://reviews.apache.org/r/41772/#comment176176>
Suggestion:
Returns a Resources object with the same amount of each resource type as these Resources. If the optional `RevocableInfo` is given, the resource's `revocable` field is set. Otherwise, the resource's `revocable` field is cleared.
src/common/resources.cpp (lines 874 - 877)
<https://reviews.apache.org/r/41772/#comment176163>
This is pretty much a copy of `Resources::reserved` now. You can remove it.
src/common/resources.cpp (line 880)
<https://reviews.apache.org/r/41772/#comment176167>
It should be fine to just name this `flatten`.
You should also consider changing the parameter type from `RevocableInfo::Type` to just `RevocableInfo`. Supplying the whole `RevocableInfo` will save us another tweak to the helpers if we change the revocable protobufs again.
- Joseph Wu
On Jan. 15, 2016, 8:23 p.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41772/
> -----------------------------------------------------------
>
> (Updated Jan. 15, 2016, 8:23 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
>
>
> Bugs: MESOS-4267
> https://issues.apache.org/jira/browse/MESOS-4267
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added helper function to flatten resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp cc8fef9470d779078aa408ed03e747e5a492deaa
> include/mesos/v1/resources.hpp f4892977f8d7b0439db6e9cf7921334f606a496c
> src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a
> src/tests/resources_tests.cpp b42610f1bf8eacfd7bf388d351f8745f1d96f666
> src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc
>
> Diff: https://reviews.apache.org/r/41772/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ResourcesOperationTest.*" --verbose
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/
-----------------------------------------------------------
(Updated 一月 16, 2016, 4:23 a.m.)
Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
Changes
-------
Rebase
Bugs: MESOS-4267
https://issues.apache.org/jira/browse/MESOS-4267
Repository: mesos
Description (updated)
-------
Added helper function to flatten resources.
Diffs (updated)
-----
include/mesos/resources.hpp cc8fef9470d779078aa408ed03e747e5a492deaa
include/mesos/v1/resources.hpp f4892977f8d7b0439db6e9cf7921334f606a496c
src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a
src/tests/resources_tests.cpp b42610f1bf8eacfd7bf388d351f8745f1d96f666
src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc
Diff: https://reviews.apache.org/r/41772/diff/
Testing
-------
make
make check
GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ResourcesOperationTest.*" --verbose
Thanks,
Guangya Liu
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/
-----------------------------------------------------------
(Updated 一月 13, 2016, 12:16 p.m.)
Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
Bugs: MESOS-4267
https://issues.apache.org/jira/browse/MESOS-4267
Repository: mesos
Description
-------
Added two new helper functions to flatten resources.
1) Flatten reserved resources.
2) Flatten allocation slack revocable resources.
Diffs (updated)
-----
include/mesos/resources.hpp cc8fef9470d779078aa408ed03e747e5a492deaa
include/mesos/v1/resources.hpp f4892977f8d7b0439db6e9cf7921334f606a496c
src/common/resources.cpp 6afd2dfd81adecf7bdb3e523bc840efb62b79ef4
src/tests/resources_tests.cpp e4a3435adc14f3b6b278b32348a6991543d5a320
src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc
Diff: https://reviews.apache.org/r/41772/diff/
Testing (updated)
-------
make
make check
GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ResourcesOperationTest.*" --verbose
Thanks,
Guangya Liu
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/
-----------------------------------------------------------
(Updated 一月 8, 2016, 6:46 a.m.)
Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
Bugs: MESOS-4267
https://issues.apache.org/jira/browse/MESOS-4267
Repository: mesos
Description
-------
Added two new helper functions to flatten resources.
1) Flatten reserved resources.
2) Flatten allocation slack revocable resources.
Diffs (updated)
-----
include/mesos/resources.hpp cc8fef9470d779078aa408ed03e747e5a492deaa
include/mesos/v1/resources.hpp f4892977f8d7b0439db6e9cf7921334f606a496c
src/common/resources.cpp 6afd2dfd81adecf7bdb3e523bc840efb62b79ef4
src/tests/resources_tests.cpp e4a3435adc14f3b6b278b32348a6991543d5a320
src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc
Diff: https://reviews.apache.org/r/41772/diff/
Testing
-------
make
make check
Thanks,
Guangya Liu
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/
-----------------------------------------------------------
(Updated 一月 7, 2016, 10:52 a.m.)
Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
Changes
-------
Rebase
Bugs: MESOS-4267
https://issues.apache.org/jira/browse/MESOS-4267
Repository: mesos
Description
-------
Added two new helper functions to flatten resources.
1) Flatten reserved resources.
2) Flatten allocation slack revocable resources.
Diffs (updated)
-----
include/mesos/resources.hpp cc8fef9470d779078aa408ed03e747e5a492deaa
include/mesos/v1/resources.hpp f4892977f8d7b0439db6e9cf7921334f606a496c
src/common/resources.cpp 6afd2dfd81adecf7bdb3e523bc840efb62b79ef4
src/tests/resources_tests.cpp e4a3435adc14f3b6b278b32348a6991543d5a320
src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc
Diff: https://reviews.apache.org/r/41772/diff/
Testing
-------
make
make check
Thanks,
Guangya Liu
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Guangya Liu <gy...@gmail.com>.
> On 一月 7, 2016, 12:31 a.m., Joseph Wu wrote:
> > src/common/resources.cpp, lines 880-891
> > <https://reviews.apache.org/r/41772/diff/2/?file=1180306#file1180306line880>
> >
> > Does this give you more value than something like this?
> > ```
> > resources.filter(isReserved).flatten()
> > ```
The reason that I want to add this helper is because I do not want to use "resources.filter(isReserved).flatten()" every time when I want to get flatten reserved, as it is a bit longer.
So what about following?
Resources Resources::reserved_() const
{
return resources.filter(isReserved).flatten();
}
This can make the caller get flatten resrved with only one call.
> On 一月 7, 2016, 12:31 a.m., Joseph Wu wrote:
> > src/common/resources.cpp, lines 894-904
> > <https://reviews.apache.org/r/41772/diff/2/?file=1180306#file1180306line894>
> >
> > This seems more appropriate as an optional parameter for `Resources::flatten`, just like `Option<Resource::ReservationInfo>`.
My original thinking was adding an optional parameter to flatten(), the reason that I did not do that is because every time I want to set ALLOCATION_SLACK or USAGE_SLACK for a resource, I need to put all paramters, it is a long sentence, so I just add this new helper.
The caller need to call {flatten("*", None(), Resource::RevocableInfo::ALLOCATION_SLACK);} if want to set a resources as allocation slack.
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/#review113153
-----------------------------------------------------------
On 一月 3, 2016, 9:42 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41772/
> -----------------------------------------------------------
>
> (Updated 一月 3, 2016, 9:42 a.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
>
>
> Bugs: MESOS-4267
> https://issues.apache.org/jira/browse/MESOS-4267
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added two new helper functions to flatten resources.
> 1) Flatten reserved resources.
> 2) Flatten allocation slack revocable resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 9c8bfb0e7e1e40ca99b4cc75079e5b950630243d
> include/mesos/v1/resources.hpp db87c992e2ec466cb1d4d17775bf6423a2fb15aa
> src/common/resources.cpp b9d31aef8babee212374e352c57fadbff02167f3
> src/tests/resources_tests.cpp e4a3435adc14f3b6b278b32348a6991543d5a320
> src/v1/resources.cpp 0c0eee6b3b7ae3254d7dc7b06187855c9b873764
>
> Diff: https://reviews.apache.org/r/41772/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Guangya Liu <gy...@gmail.com>.
> On 一月 7, 2016, 12:31 a.m., Joseph Wu wrote:
> > src/common/resources.cpp, lines 880-891
> > <https://reviews.apache.org/r/41772/diff/2/?file=1180306#file1180306line880>
> >
> > Does this give you more value than something like this?
> > ```
> > resources.filter(isReserved).flatten()
> > ```
>
> Guangya Liu wrote:
> The reason that I want to add this helper is because I do not want to use "resources.filter(isReserved).flatten()" every time when I want to get flatten reserved, as it is a bit longer.
>
> So what about following?
>
> Resources Resources::reserved_() const
> {
> return resources.filter(isReserved).flatten();
> }
>
> This can make the caller get flatten resrved with only one call.
>
> Joseph Wu wrote:
> I don't think a slight shortening is worth the extra helper method.
Joseph, the total sentense would be as following if we want to get the flatten reserved resources.
`Resources flattenReserved1 = resources.filter(lambda::bind(Resources::isReserved, lambda::_1, None())).flatten();`
The current API reserved() will return a map of reserved resources, I think it is good to add another API to return all reserved resources in flatten mode. comments?
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/#review113153
-----------------------------------------------------------
On 一月 7, 2016, 10:52 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41772/
> -----------------------------------------------------------
>
> (Updated 一月 7, 2016, 10:52 a.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
>
>
> Bugs: MESOS-4267
> https://issues.apache.org/jira/browse/MESOS-4267
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added two new helper functions to flatten resources.
> 1) Flatten reserved resources.
> 2) Flatten allocation slack revocable resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp cc8fef9470d779078aa408ed03e747e5a492deaa
> include/mesos/v1/resources.hpp f4892977f8d7b0439db6e9cf7921334f606a496c
> src/common/resources.cpp 6afd2dfd81adecf7bdb3e523bc840efb62b79ef4
> src/tests/resources_tests.cpp e4a3435adc14f3b6b278b32348a6991543d5a320
> src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc
>
> Diff: https://reviews.apache.org/r/41772/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Jan. 6, 2016, 4:31 p.m., Joseph Wu wrote:
> > src/common/resources.cpp, lines 880-891
> > <https://reviews.apache.org/r/41772/diff/2/?file=1180306#file1180306line880>
> >
> > Does this give you more value than something like this?
> > ```
> > resources.filter(isReserved).flatten()
> > ```
>
> Guangya Liu wrote:
> The reason that I want to add this helper is because I do not want to use "resources.filter(isReserved).flatten()" every time when I want to get flatten reserved, as it is a bit longer.
>
> So what about following?
>
> Resources Resources::reserved_() const
> {
> return resources.filter(isReserved).flatten();
> }
>
> This can make the caller get flatten resrved with only one call.
I don't think a slight shortening is worth the extra helper method.
> On Jan. 6, 2016, 4:31 p.m., Joseph Wu wrote:
> > src/common/resources.cpp, lines 894-904
> > <https://reviews.apache.org/r/41772/diff/2/?file=1180306#file1180306line894>
> >
> > This seems more appropriate as an optional parameter for `Resources::flatten`, just like `Option<Resource::ReservationInfo>`.
>
> Guangya Liu wrote:
> My original thinking was adding an optional parameter to flatten(), the reason that I did not do that is because every time I want to set ALLOCATION_SLACK or USAGE_SLACK for a resource, I need to put all paramters, it is a long sentence, so I just add this new helper.
>
> The caller need to call {flatten("*", None(), Resource::RevocableInfo::ALLOCATION_SLACK);} if want to set a resources as allocation slack.
It's fine to have somewhat long expressions. I think it's preferable over having too many helpers.
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/#review113153
-----------------------------------------------------------
On Jan. 7, 2016, 2:52 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41772/
> -----------------------------------------------------------
>
> (Updated Jan. 7, 2016, 2:52 a.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
>
>
> Bugs: MESOS-4267
> https://issues.apache.org/jira/browse/MESOS-4267
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added two new helper functions to flatten resources.
> 1) Flatten reserved resources.
> 2) Flatten allocation slack revocable resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp cc8fef9470d779078aa408ed03e747e5a492deaa
> include/mesos/v1/resources.hpp f4892977f8d7b0439db6e9cf7921334f606a496c
> src/common/resources.cpp 6afd2dfd81adecf7bdb3e523bc840efb62b79ef4
> src/tests/resources_tests.cpp e4a3435adc14f3b6b278b32348a6991543d5a320
> src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc
>
> Diff: https://reviews.apache.org/r/41772/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Klaus Ma <kl...@gmail.com>.
> On Jan. 7, 2016, 8:31 a.m., Joseph Wu wrote:
> > src/common/resources.cpp, lines 894-904
> > <https://reviews.apache.org/r/41772/diff/2/?file=1180306#file1180306line894>
> >
> > This seems more appropriate as an optional parameter for `Resources::flatten`, just like `Option<Resource::ReservationInfo>`.
>
> Guangya Liu wrote:
> My original thinking was adding an optional parameter to flatten(), the reason that I did not do that is because every time I want to set ALLOCATION_SLACK or USAGE_SLACK for a resource, I need to put all paramters, it is a long sentence, so I just add this new helper.
>
> The caller need to call {flatten("*", None(), Resource::RevocableInfo::ALLOCATION_SLACK);} if want to set a resources as allocation slack.
>
> Joseph Wu wrote:
> It's fine to have somewhat long expressions. I think it's preferable over having too many helpers.
>
> Guangya Liu wrote:
> @Joseph, seems we need to reach an agreement for this ;-) Can you please take a look at the following patches in the patch chain, you may notice that other patches highly depend on those helper functions.
>
> So what about the following:
> 1) Rename flattenReserved() to reserved_() which will return all reserved resources with different roles in a flatten mode.
> 2) Remove flattenSlack() and merge it to flatten().
>
> Comments?
>
> Klaus Ma wrote:
> regarding 1), I'd like to add `allocationSlackable` which mean the reosurces can be offered as `ALLOCATION_SLACK`, it's peer of `allocationSlack`.
>
> Guangya Liu wrote:
> `allcationSlackable` is more like a `bool` function, but here I want to return a `flatten reserved` resources.
It's not about `bool` :). I'd like to avoid the backgroud of `ALLOCATION_SLACK-able` == `stateless.reserved`: the general point is `ALLOCATION_SLACK-able` resources can be lend to other Tenant framework, and those resource will be preempted when Lender framework launch tasks. That's major reason that I'd suggest to introduce helper fuction to avoid `stateless.reserved` in Master/Agent source code.
En, maybe this can be treat as new MVP named borrow/lend policy worked with Quota; it's different with Optimistic Offer Phase 2, the revocalbe resources will not send to multiple framework; the revocable resources will be shared between frameworks by ratio. @Joseph/Ben, any comments.
- Klaus
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/#review113153
-----------------------------------------------------------
On Jan. 13, 2016, 8:16 p.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41772/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2016, 8:16 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
>
>
> Bugs: MESOS-4267
> https://issues.apache.org/jira/browse/MESOS-4267
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added two new helper functions to flatten resources.
> 1) Flatten reserved resources.
> 2) Flatten allocation slack revocable resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp cc8fef9470d779078aa408ed03e747e5a492deaa
> include/mesos/v1/resources.hpp f4892977f8d7b0439db6e9cf7921334f606a496c
> src/common/resources.cpp 6afd2dfd81adecf7bdb3e523bc840efb62b79ef4
> src/tests/resources_tests.cpp e4a3435adc14f3b6b278b32348a6991543d5a320
> src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc
>
> Diff: https://reviews.apache.org/r/41772/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ResourcesOperationTest.*" --verbose
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Guangya Liu <gy...@gmail.com>.
> On 一月 7, 2016, 12:31 a.m., Joseph Wu wrote:
> > src/common/resources.cpp, lines 894-904
> > <https://reviews.apache.org/r/41772/diff/2/?file=1180306#file1180306line894>
> >
> > This seems more appropriate as an optional parameter for `Resources::flatten`, just like `Option<Resource::ReservationInfo>`.
>
> Guangya Liu wrote:
> My original thinking was adding an optional parameter to flatten(), the reason that I did not do that is because every time I want to set ALLOCATION_SLACK or USAGE_SLACK for a resource, I need to put all paramters, it is a long sentence, so I just add this new helper.
>
> The caller need to call {flatten("*", None(), Resource::RevocableInfo::ALLOCATION_SLACK);} if want to set a resources as allocation slack.
>
> Joseph Wu wrote:
> It's fine to have somewhat long expressions. I think it's preferable over having too many helpers.
>
> Guangya Liu wrote:
> @Joseph, seems we need to reach an agreement for this ;-) Can you please take a look at the following patches in the patch chain, you may notice that other patches highly depend on those helper functions.
>
> So what about the following:
> 1) Rename flattenReserved() to reserved_() which will return all reserved resources with different roles in a flatten mode.
> 2) Remove flattenSlack() and merge it to flatten().
>
> Comments?
>
> Klaus Ma wrote:
> regarding 1), I'd like to add `allocationSlackable` which mean the reosurces can be offered as `ALLOCATION_SLACK`, it's peer of `allocationSlack`.
`allcationSlackable` is more like a `bool` function, but here I want to return a `flatten reserved` resources.
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/#review113153
-----------------------------------------------------------
On 一月 13, 2016, 12:16 p.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41772/
> -----------------------------------------------------------
>
> (Updated 一月 13, 2016, 12:16 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
>
>
> Bugs: MESOS-4267
> https://issues.apache.org/jira/browse/MESOS-4267
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added two new helper functions to flatten resources.
> 1) Flatten reserved resources.
> 2) Flatten allocation slack revocable resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp cc8fef9470d779078aa408ed03e747e5a492deaa
> include/mesos/v1/resources.hpp f4892977f8d7b0439db6e9cf7921334f606a496c
> src/common/resources.cpp 6afd2dfd81adecf7bdb3e523bc840efb62b79ef4
> src/tests/resources_tests.cpp e4a3435adc14f3b6b278b32348a6991543d5a320
> src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc
>
> Diff: https://reviews.apache.org/r/41772/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ResourcesOperationTest.*" --verbose
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Klaus Ma <kl...@gmail.com>.
> On Jan. 7, 2016, 8:31 a.m., Joseph Wu wrote:
> > src/common/resources.cpp, lines 894-904
> > <https://reviews.apache.org/r/41772/diff/2/?file=1180306#file1180306line894>
> >
> > This seems more appropriate as an optional parameter for `Resources::flatten`, just like `Option<Resource::ReservationInfo>`.
>
> Guangya Liu wrote:
> My original thinking was adding an optional parameter to flatten(), the reason that I did not do that is because every time I want to set ALLOCATION_SLACK or USAGE_SLACK for a resource, I need to put all paramters, it is a long sentence, so I just add this new helper.
>
> The caller need to call {flatten("*", None(), Resource::RevocableInfo::ALLOCATION_SLACK);} if want to set a resources as allocation slack.
>
> Joseph Wu wrote:
> It's fine to have somewhat long expressions. I think it's preferable over having too many helpers.
>
> Guangya Liu wrote:
> @Joseph, seems we need to reach an agreement for this ;-) Can you please take a look at the following patches in the patch chain, you may notice that other patches highly depend on those helper functions.
>
> So what about the following:
> 1) Rename flattenReserved() to reserved_() which will return all reserved resources with different roles in a flatten mode.
> 2) Remove flattenSlack() and merge it to flatten().
>
> Comments?
>
> Klaus Ma wrote:
> regarding 1), I'd like to add `allocationSlackable` which mean the reosurces can be offered as `ALLOCATION_SLACK`, it's peer of `allocationSlack`.
>
> Guangya Liu wrote:
> `allcationSlackable` is more like a `bool` function, but here I want to return a `flatten reserved` resources.
>
> Klaus Ma wrote:
> It's not about `bool` :). I'd like to avoid the backgroud of `ALLOCATION_SLACK-able` == `stateless.reserved`: the general point is `ALLOCATION_SLACK-able` resources can be lend to other Tenant framework, and those resource will be preempted when Lender framework launch tasks. That's major reason that I'd suggest to introduce helper fuction to avoid `stateless.reserved` in Master/Agent source code.
>
> En, maybe this can be treat as new MVP named borrow/lend policy worked with Quota; it's different with Optimistic Offer Phase 2, the revocalbe resources will not send to multiple framework; the revocable resources will be shared between frameworks by ratio. @Joseph/Ben, any comments.
>
> Guangya Liu wrote:
> I think we did discuss this problem when start coding for phase 1, the phase 1 only consider the allocation slack which is get from unused reserved resources, but Quota is not using reserved resources but only unreserved non revocable resources. I think that we can put this to post MVP and write a new document for this and clarify the behavior there. Comments?
Yes, that's why I call it "new MVP" :). It seems similar proposal with MESOS-4392, I'll append my comments to MESOS-4392.
- Klaus
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/#review113153
-----------------------------------------------------------
On Jan. 16, 2016, 12:23 p.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41772/
> -----------------------------------------------------------
>
> (Updated Jan. 16, 2016, 12:23 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
>
>
> Bugs: MESOS-4267
> https://issues.apache.org/jira/browse/MESOS-4267
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added helper function to flatten resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp cc8fef9470d779078aa408ed03e747e5a492deaa
> include/mesos/v1/resources.hpp f4892977f8d7b0439db6e9cf7921334f606a496c
> src/common/resources.cpp 575d6651185d8431f01d589f4afc255cb751181a
> src/tests/resources_tests.cpp b42610f1bf8eacfd7bf388d351f8745f1d96f666
> src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc
>
> Diff: https://reviews.apache.org/r/41772/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ResourcesOperationTest.*" --verbose
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Guangya Liu <gy...@gmail.com>.
> On 一月 7, 2016, 12:31 a.m., Joseph Wu wrote:
> > src/common/resources.cpp, lines 894-904
> > <https://reviews.apache.org/r/41772/diff/2/?file=1180306#file1180306line894>
> >
> > This seems more appropriate as an optional parameter for `Resources::flatten`, just like `Option<Resource::ReservationInfo>`.
>
> Guangya Liu wrote:
> My original thinking was adding an optional parameter to flatten(), the reason that I did not do that is because every time I want to set ALLOCATION_SLACK or USAGE_SLACK for a resource, I need to put all paramters, it is a long sentence, so I just add this new helper.
>
> The caller need to call {flatten("*", None(), Resource::RevocableInfo::ALLOCATION_SLACK);} if want to set a resources as allocation slack.
>
> Joseph Wu wrote:
> It's fine to have somewhat long expressions. I think it's preferable over having too many helpers.
@Joseph, seems we need to reach an agreement for this ;-) Can you please take a look at the following patches in the patch chain, you may notice that other patches highly depend on those helper functions.
So what about the following:
1) Rename flattenReserved() to reserved_() which will return all reserved resources with different roles in a flatten mode.
2) Remove flattenSlack() and merge it to flatten().
Comments?
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/#review113153
-----------------------------------------------------------
On 一月 13, 2016, 12:16 p.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41772/
> -----------------------------------------------------------
>
> (Updated 一月 13, 2016, 12:16 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
>
>
> Bugs: MESOS-4267
> https://issues.apache.org/jira/browse/MESOS-4267
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added two new helper functions to flatten resources.
> 1) Flatten reserved resources.
> 2) Flatten allocation slack revocable resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp cc8fef9470d779078aa408ed03e747e5a492deaa
> include/mesos/v1/resources.hpp f4892977f8d7b0439db6e9cf7921334f606a496c
> src/common/resources.cpp 6afd2dfd81adecf7bdb3e523bc840efb62b79ef4
> src/tests/resources_tests.cpp e4a3435adc14f3b6b278b32348a6991543d5a320
> src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc
>
> Diff: https://reviews.apache.org/r/41772/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ResourcesOperationTest.*" --verbose
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Klaus Ma <kl...@gmail.com>.
> On Jan. 7, 2016, 8:31 a.m., Joseph Wu wrote:
> > src/common/resources.cpp, lines 894-904
> > <https://reviews.apache.org/r/41772/diff/2/?file=1180306#file1180306line894>
> >
> > This seems more appropriate as an optional parameter for `Resources::flatten`, just like `Option<Resource::ReservationInfo>`.
>
> Guangya Liu wrote:
> My original thinking was adding an optional parameter to flatten(), the reason that I did not do that is because every time I want to set ALLOCATION_SLACK or USAGE_SLACK for a resource, I need to put all paramters, it is a long sentence, so I just add this new helper.
>
> The caller need to call {flatten("*", None(), Resource::RevocableInfo::ALLOCATION_SLACK);} if want to set a resources as allocation slack.
>
> Joseph Wu wrote:
> It's fine to have somewhat long expressions. I think it's preferable over having too many helpers.
>
> Guangya Liu wrote:
> @Joseph, seems we need to reach an agreement for this ;-) Can you please take a look at the following patches in the patch chain, you may notice that other patches highly depend on those helper functions.
>
> So what about the following:
> 1) Rename flattenReserved() to reserved_() which will return all reserved resources with different roles in a flatten mode.
> 2) Remove flattenSlack() and merge it to flatten().
>
> Comments?
regarding 1), I'd like to add `allocationSlackable` which mean the reosurces can be offered as `ALLOCATION_SLACK`, it's peer of `allocationSlack`.
- Klaus
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/#review113153
-----------------------------------------------------------
On Jan. 13, 2016, 8:16 p.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41772/
> -----------------------------------------------------------
>
> (Updated Jan. 13, 2016, 8:16 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
>
>
> Bugs: MESOS-4267
> https://issues.apache.org/jira/browse/MESOS-4267
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added two new helper functions to flatten resources.
> 1) Flatten reserved resources.
> 2) Flatten allocation slack revocable resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp cc8fef9470d779078aa408ed03e747e5a492deaa
> include/mesos/v1/resources.hpp f4892977f8d7b0439db6e9cf7921334f606a496c
> src/common/resources.cpp 6afd2dfd81adecf7bdb3e523bc840efb62b79ef4
> src/tests/resources_tests.cpp e4a3435adc14f3b6b278b32348a6991543d5a320
> src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc
>
> Diff: https://reviews.apache.org/r/41772/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ResourcesOperationTest.*" --verbose
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Guangya Liu <gy...@gmail.com>.
> On 一月 7, 2016, 12:31 a.m., Joseph Wu wrote:
> > src/common/resources.cpp, lines 894-904
> > <https://reviews.apache.org/r/41772/diff/2/?file=1180306#file1180306line894>
> >
> > This seems more appropriate as an optional parameter for `Resources::flatten`, just like `Option<Resource::ReservationInfo>`.
>
> Guangya Liu wrote:
> My original thinking was adding an optional parameter to flatten(), the reason that I did not do that is because every time I want to set ALLOCATION_SLACK or USAGE_SLACK for a resource, I need to put all paramters, it is a long sentence, so I just add this new helper.
>
> The caller need to call {flatten("*", None(), Resource::RevocableInfo::ALLOCATION_SLACK);} if want to set a resources as allocation slack.
>
> Joseph Wu wrote:
> It's fine to have somewhat long expressions. I think it's preferable over having too many helpers.
>
> Guangya Liu wrote:
> @Joseph, seems we need to reach an agreement for this ;-) Can you please take a look at the following patches in the patch chain, you may notice that other patches highly depend on those helper functions.
>
> So what about the following:
> 1) Rename flattenReserved() to reserved_() which will return all reserved resources with different roles in a flatten mode.
> 2) Remove flattenSlack() and merge it to flatten().
>
> Comments?
>
> Klaus Ma wrote:
> regarding 1), I'd like to add `allocationSlackable` which mean the reosurces can be offered as `ALLOCATION_SLACK`, it's peer of `allocationSlack`.
>
> Guangya Liu wrote:
> `allcationSlackable` is more like a `bool` function, but here I want to return a `flatten reserved` resources.
>
> Klaus Ma wrote:
> It's not about `bool` :). I'd like to avoid the backgroud of `ALLOCATION_SLACK-able` == `stateless.reserved`: the general point is `ALLOCATION_SLACK-able` resources can be lend to other Tenant framework, and those resource will be preempted when Lender framework launch tasks. That's major reason that I'd suggest to introduce helper fuction to avoid `stateless.reserved` in Master/Agent source code.
>
> En, maybe this can be treat as new MVP named borrow/lend policy worked with Quota; it's different with Optimistic Offer Phase 2, the revocalbe resources will not send to multiple framework; the revocable resources will be shared between frameworks by ratio. @Joseph/Ben, any comments.
I think we did discuss this problem when start coding for phase 1, the phase 1 only consider the allocation slack which is get from unused reserved resources, but Quota is not using reserved resources but only unreserved non revocable resources. I think that we can put this to post MVP and write a new document for this and clarify the behavior there. Comments?
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/#review113153
-----------------------------------------------------------
On 一月 13, 2016, 12:16 p.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41772/
> -----------------------------------------------------------
>
> (Updated 一月 13, 2016, 12:16 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
>
>
> Bugs: MESOS-4267
> https://issues.apache.org/jira/browse/MESOS-4267
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added two new helper functions to flatten resources.
> 1) Flatten reserved resources.
> 2) Flatten allocation slack revocable resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp cc8fef9470d779078aa408ed03e747e5a492deaa
> include/mesos/v1/resources.hpp f4892977f8d7b0439db6e9cf7921334f606a496c
> src/common/resources.cpp 6afd2dfd81adecf7bdb3e523bc840efb62b79ef4
> src/tests/resources_tests.cpp e4a3435adc14f3b6b278b32348a6991543d5a320
> src/v1/resources.cpp 8de6672ba9b34947db81c74b8e03e8965e8af5fc
>
> Diff: https://reviews.apache.org/r/41772/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ResourcesOperationTest.*" --verbose
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/#review113153
-----------------------------------------------------------
include/mesos/resources.hpp (lines 267 - 275)
<https://reviews.apache.org/r/41772/#comment173656>
I'd recommend killing these. See issues below.
src/common/resources.cpp (lines 880 - 891)
<https://reviews.apache.org/r/41772/#comment173652>
Does this give you more value than something like this?
```
resources.filter(isReserved).flatten()
```
src/common/resources.cpp (lines 894 - 904)
<https://reviews.apache.org/r/41772/#comment173654>
This seems more appropriate as an optional parameter for `Resources::flatten`, just like `Option<Resource::ReservationInfo>`.
- Joseph Wu
On Jan. 3, 2016, 1:42 a.m., Guangya Liu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41772/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2016, 1:42 a.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
>
>
> Bugs: MESOS-4267
> https://issues.apache.org/jira/browse/MESOS-4267
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added two new helper functions to flatten resources.
> 1) Flatten reserved resources.
> 2) Flatten allocation slack revocable resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 9c8bfb0e7e1e40ca99b4cc75079e5b950630243d
> include/mesos/v1/resources.hpp db87c992e2ec466cb1d4d17775bf6423a2fb15aa
> src/common/resources.cpp b9d31aef8babee212374e352c57fadbff02167f3
> src/tests/resources_tests.cpp e4a3435adc14f3b6b278b32348a6991543d5a320
> src/v1/resources.cpp 0c0eee6b3b7ae3254d7dc7b06187855c9b873764
>
> Diff: https://reviews.apache.org/r/41772/diff/
>
>
> Testing
> -------
>
> make
> make check
>
>
> Thanks,
>
> Guangya Liu
>
>
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/
-----------------------------------------------------------
(Updated 一月 3, 2016, 9:42 a.m.)
Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
Bugs: MESOS-4267
https://issues.apache.org/jira/browse/MESOS-4267
Repository: mesos
Description
-------
Added two new helper functions to flatten resources.
1) Flatten reserved resources.
2) Flatten allocation slack revocable resources.
Diffs
-----
include/mesos/resources.hpp 9c8bfb0e7e1e40ca99b4cc75079e5b950630243d
include/mesos/v1/resources.hpp db87c992e2ec466cb1d4d17775bf6423a2fb15aa
src/common/resources.cpp b9d31aef8babee212374e352c57fadbff02167f3
src/tests/resources_tests.cpp e4a3435adc14f3b6b278b32348a6991543d5a320
src/v1/resources.cpp 0c0eee6b3b7ae3254d7dc7b06187855c9b873764
Diff: https://reviews.apache.org/r/41772/diff/
Testing
-------
make
make check
Thanks,
Guangya Liu
Re: Review Request 41772: Added helper function to flatten resources.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41772/
-----------------------------------------------------------
(Updated 一月 2, 2016, 1:16 a.m.)
Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, and Klaus Ma.
Summary (updated)
-----------------
Added helper function to flatten resources.
Repository: mesos
Description
-------
Added two new helper functions to flatten resources.
1) Flatten reserved resources.
2) Flatten allocation slack revocable resources.
Diffs
-----
include/mesos/resources.hpp 9c8bfb0e7e1e40ca99b4cc75079e5b950630243d
include/mesos/v1/resources.hpp db87c992e2ec466cb1d4d17775bf6423a2fb15aa
src/common/resources.cpp b9d31aef8babee212374e352c57fadbff02167f3
src/tests/resources_tests.cpp e4a3435adc14f3b6b278b32348a6991543d5a320
src/v1/resources.cpp 0c0eee6b3b7ae3254d7dc7b06187855c9b873764
Diff: https://reviews.apache.org/r/41772/diff/
Testing
-------
make
make check
Thanks,
Guangya Liu