You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Mahler <bm...@apache.org> on 2016/12/17 22:50:38 UTC
Review Request 54836: Added helpers to allocate / unallocate
Resources.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54836/
-----------------------------------------------------------
Review request for mesos, Benjamin Bannier, Jay Guo, and Guangya Liu.
Repository: mesos
Description
-------
Added helpers to allocate / unallocate Resources.
Diffs
-----
include/mesos/resources.hpp c2e6d6680d9687f0bbfb73e84ffb1457072c4412
include/mesos/v1/resources.hpp 98f025b68c818c384a1174ef9b969f5c0776a7f1
src/common/resources.cpp 16f6de6741b548a8a9fff5b70447b1a312f10efe
src/tests/resources_tests.cpp c0ff9777ceb1731086179e2966b07c46d035c110
src/v1/resources.cpp a1adf1b27964bf6a89d97715cff05878d8c6cae9
Diff: https://reviews.apache.org/r/54836/diff/
Testing
-------
Updated the existing allocation test to incorporate the new helper.
Thanks,
Benjamin Mahler
Re: Review Request 54836: Added helpers to allocate / unallocate
Resources.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Jan. 25, 2017, 10:40 a.m., Guangya Liu wrote:
> > src/tests/resources_tests.cpp, line 2774
> > <https://reviews.apache.org/r/54836/diff/3/?file=1611728#file1611728line2774>
> >
> > How about
> >
> > ```
> > Resources cpus2 = cpus1;
> > ```
> >
> > Ditto for others.
Hm.. I found this makes the test a bit harder to follow, since you have to look at the other definition to see the value. Note that we don't care here that cpus2 is equal to cpus1, just that cpus2 is unreserved.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54836/#review162937
-----------------------------------------------------------
On Jan. 23, 2017, 1:55 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54836/
> -----------------------------------------------------------
>
> (Updated Jan. 23, 2017, 1:55 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added helpers to allocate / unallocate Resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp c2e6d6680d9687f0bbfb73e84ffb1457072c4412
> include/mesos/v1/resources.hpp 98f025b68c818c384a1174ef9b969f5c0776a7f1
> src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45
> src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68
> src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267
>
> Diff: https://reviews.apache.org/r/54836/diff/
>
>
> Testing
> -------
>
> Updated the existing allocation test to incorporate the new helper.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 54836: Added helpers to allocate / unallocate
Resources.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54836/#review162937
-----------------------------------------------------------
src/tests/resources_tests.cpp (line 2759)
<https://reviews.apache.org/r/54836/#comment234329>
How about
```
Resources cpus2 = cpus1;
```
Ditto for others.
- Guangya Liu
On \u4e00\u6708 23, 2017, 1:55 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54836/
> -----------------------------------------------------------
>
> (Updated \u4e00\u6708 23, 2017, 1:55 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added helpers to allocate / unallocate Resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp c2e6d6680d9687f0bbfb73e84ffb1457072c4412
> include/mesos/v1/resources.hpp 98f025b68c818c384a1174ef9b969f5c0776a7f1
> src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45
> src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68
> src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267
>
> Diff: https://reviews.apache.org/r/54836/diff/
>
>
> Testing
> -------
>
> Updated the existing allocation test to incorporate the new helper.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 54836: Added helpers to allocate / unallocate
Resources.
Posted by Michael Park <mp...@apache.org>.
> On Jan. 25, 2017, 1:54 a.m., Michael Park wrote:
> > include/mesos/resources.hpp, lines 332-339
> > <https://reviews.apache.org/r/54836/diff/3/?file=1611725#file1611725line332>
> >
> > I would've expected:
> > ```cpp
> > void allocate(const std::string& role);
> > void unallocate();
> > ```
> >
> > Could you help me understand what the `bool` from `unallocate` is for?
> >
> > If it's simply to indicate whether the function had any effects or
> > not, `allocate` could also return a `bool`, which returns `false`
> > when the resources were already allocated for the specified role.
>
> Guangya Liu wrote:
> For `allocate`, seems the question here is same as mine above: In which case shall we need to overwrite the role for allocation?
>
> Guangya Liu wrote:
> @mpark, I think that Ben already posted some comments here for why return `bool` for `unallocate` at https://reviews.apache.org/r/54836/#comment230598
@bmahler, Actually, upon looking further in the chain I realized that mutating the resources here is new...
You had mentioned this to me as well and explained why (I think), could you explain again here
why we didn't do `allocations` (which we already have), `allocated` and `unallocated`?
Similar to `reservations`, `reserved` and `unreserved`?
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54836/#review162927
-----------------------------------------------------------
On Jan. 22, 2017, 5:55 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54836/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2017, 5:55 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added helpers to allocate / unallocate Resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp c2e6d6680d9687f0bbfb73e84ffb1457072c4412
> include/mesos/v1/resources.hpp 98f025b68c818c384a1174ef9b969f5c0776a7f1
> src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45
> src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68
> src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267
>
> Diff: https://reviews.apache.org/r/54836/diff/
>
>
> Testing
> -------
>
> Updated the existing allocation test to incorporate the new helper.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 54836: Added helpers to allocate / unallocate
Resources.
Posted by Michael Park <mp...@apache.org>.
> On Jan. 25, 2017, 1:54 a.m., Michael Park wrote:
> > include/mesos/resources.hpp, lines 332-339
> > <https://reviews.apache.org/r/54836/diff/3/?file=1611725#file1611725line332>
> >
> > I would've expected:
> > ```cpp
> > void allocate(const std::string& role);
> > void unallocate();
> > ```
> >
> > Could you help me understand what the `bool` from `unallocate` is for?
> >
> > If it's simply to indicate whether the function had any effects or
> > not, `allocate` could also return a `bool`, which returns `false`
> > when the resources were already allocated for the specified role.
>
> Guangya Liu wrote:
> For `allocate`, seems the question here is same as mine above: In which case shall we need to overwrite the role for allocation?
>
> Guangya Liu wrote:
> @mpark, I think that Ben already posted some comments here for why return `bool` for `unallocate` at https://reviews.apache.org/r/54836/#comment230598
>
> Michael Park wrote:
> @bmahler, Actually, upon looking further in the chain I realized that mutating the resources here is new...
> You had mentioned this to me as well and explained why (I think), could you explain again here
> why we didn't do `allocations` (which we already have), `allocated` and `unallocated`?
> Similar to `reservations`, `reserved` and `unreserved`?
Never mind, regarding my last comment. I had another realization that we're not trying
to filter for `allocated` and `unallocated` resources, but rather want to mutate them.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54836/#review162927
-----------------------------------------------------------
On Jan. 22, 2017, 5:55 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54836/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2017, 5:55 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added helpers to allocate / unallocate Resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp c2e6d6680d9687f0bbfb73e84ffb1457072c4412
> include/mesos/v1/resources.hpp 98f025b68c818c384a1174ef9b969f5c0776a7f1
> src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45
> src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68
> src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267
>
> Diff: https://reviews.apache.org/r/54836/diff/
>
>
> Testing
> -------
>
> Updated the existing allocation test to incorporate the new helper.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 54836: Added helpers to allocate / unallocate
Resources.
Posted by Guangya Liu <gy...@gmail.com>.
> On \u4e00\u6708 25, 2017, 9:54 a.m., Michael Park wrote:
> > include/mesos/resources.hpp, lines 332-339
> > <https://reviews.apache.org/r/54836/diff/3/?file=1611725#file1611725line332>
> >
> > I would've expected:
> > ```cpp
> > void allocate(const std::string& role);
> > void unallocate();
> > ```
> >
> > Could you help me understand what the `bool` from `unallocate` is for?
> >
> > If it's simply to indicate whether the function had any effects or
> > not, `allocate` could also return a `bool`, which returns `false`
> > when the resources were already allocated for the specified role.
For `allocate`, seems the question here is same as mine above: In which case shall we need to overwrite the role for allocation?
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54836/#review162927
-----------------------------------------------------------
On \u4e00\u6708 23, 2017, 1:55 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54836/
> -----------------------------------------------------------
>
> (Updated \u4e00\u6708 23, 2017, 1:55 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added helpers to allocate / unallocate Resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp c2e6d6680d9687f0bbfb73e84ffb1457072c4412
> include/mesos/v1/resources.hpp 98f025b68c818c384a1174ef9b969f5c0776a7f1
> src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45
> src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68
> src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267
>
> Diff: https://reviews.apache.org/r/54836/diff/
>
>
> Testing
> -------
>
> Updated the existing allocation test to incorporate the new helper.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 54836: Added helpers to allocate / unallocate
Resources.
Posted by Guangya Liu <gy...@gmail.com>.
> On \u4e00\u6708 25, 2017, 9:54 a.m., Michael Park wrote:
> > include/mesos/resources.hpp, lines 332-339
> > <https://reviews.apache.org/r/54836/diff/3/?file=1611725#file1611725line332>
> >
> > I would've expected:
> > ```cpp
> > void allocate(const std::string& role);
> > void unallocate();
> > ```
> >
> > Could you help me understand what the `bool` from `unallocate` is for?
> >
> > If it's simply to indicate whether the function had any effects or
> > not, `allocate` could also return a `bool`, which returns `false`
> > when the resources were already allocated for the specified role.
>
> Guangya Liu wrote:
> For `allocate`, seems the question here is same as mine above: In which case shall we need to overwrite the role for allocation?
@mpark, I think that Ben already posted some comments here for why return `bool` for `unallocate` at https://reviews.apache.org/r/54836/#comment230598
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54836/#review162927
-----------------------------------------------------------
On \u4e00\u6708 23, 2017, 1:55 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54836/
> -----------------------------------------------------------
>
> (Updated \u4e00\u6708 23, 2017, 1:55 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added helpers to allocate / unallocate Resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp c2e6d6680d9687f0bbfb73e84ffb1457072c4412
> include/mesos/v1/resources.hpp 98f025b68c818c384a1174ef9b969f5c0776a7f1
> src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45
> src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68
> src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267
>
> Diff: https://reviews.apache.org/r/54836/diff/
>
>
> Testing
> -------
>
> Updated the existing allocation test to incorporate the new helper.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 54836: Added helpers to allocate / unallocate
Resources.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54836/#review162927
-----------------------------------------------------------
Fix it, then Ship it!
include/mesos/resources.hpp (lines 332 - 339)
<https://reviews.apache.org/r/54836/#comment234325>
I would've expected:
```cpp
void allocate(const std::string& role);
void unallocate();
```
Could you help me understand what the `bool` from `unallocate` is for?
If it's simply to indicate whether the function had any effects or
not, `allocate` could also return a `bool`, which returns `false`
when the resources were already allocated for the specified role.
- Michael Park
On Jan. 22, 2017, 5:55 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54836/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2017, 5:55 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added helpers to allocate / unallocate Resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp c2e6d6680d9687f0bbfb73e84ffb1457072c4412
> include/mesos/v1/resources.hpp 98f025b68c818c384a1174ef9b969f5c0776a7f1
> src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45
> src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68
> src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267
>
> Diff: https://reviews.apache.org/r/54836/diff/
>
>
> Testing
> -------
>
> Updated the existing allocation test to incorporate the new helper.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 54836: Added helpers to allocate / unallocate
Resources.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54836/
-----------------------------------------------------------
(Updated Jan. 23, 2017, 1:55 a.m.)
Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael Park.
Changes
-------
Updated per review.
Repository: mesos
Description
-------
Added helpers to allocate / unallocate Resources.
Diffs (updated)
-----
include/mesos/resources.hpp c2e6d6680d9687f0bbfb73e84ffb1457072c4412
include/mesos/v1/resources.hpp 98f025b68c818c384a1174ef9b969f5c0776a7f1
src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45
src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68
src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267
Diff: https://reviews.apache.org/r/54836/diff/
Testing
-------
Updated the existing allocation test to incorporate the new helper.
Thanks,
Benjamin Mahler
Re: Review Request 54836: Added helpers to allocate / unallocate
Resources.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54836/
-----------------------------------------------------------
(Updated Dec. 18, 2016, 4:36 p.m.)
Review request for mesos, Benjamin Bannier, Jay Guo, and Guangya Liu.
Changes
-------
Updated the comments.
Repository: mesos
Description
-------
Added helpers to allocate / unallocate Resources.
Diffs (updated)
-----
include/mesos/resources.hpp c2e6d6680d9687f0bbfb73e84ffb1457072c4412
include/mesos/v1/resources.hpp 98f025b68c818c384a1174ef9b969f5c0776a7f1
src/common/resources.cpp 7dbeefc8fc159f28dc1542ad6a761213c65ff226
src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68
src/v1/resources.cpp 859f7388035b855bf0cb60d5cbe746e6871bf833
Diff: https://reviews.apache.org/r/54836/diff/
Testing
-------
Updated the existing allocation test to incorporate the new helper.
Thanks,
Benjamin Mahler
Re: Review Request 54836: Added helpers to allocate / unallocate
Resources.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54836/#review159561
-----------------------------------------------------------
Patch looks great!
Reviews applied: [54842, 54836]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On Dec. 17, 2016, 10:50 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54836/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2016, 10:50 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Jay Guo, and Guangya Liu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added helpers to allocate / unallocate Resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp c2e6d6680d9687f0bbfb73e84ffb1457072c4412
> include/mesos/v1/resources.hpp 98f025b68c818c384a1174ef9b969f5c0776a7f1
> src/common/resources.cpp 16f6de6741b548a8a9fff5b70447b1a312f10efe
> src/tests/resources_tests.cpp c0ff9777ceb1731086179e2966b07c46d035c110
> src/v1/resources.cpp a1adf1b27964bf6a89d97715cff05878d8c6cae9
>
> Diff: https://reviews.apache.org/r/54836/diff/
>
>
> Testing
> -------
>
> Updated the existing allocation test to incorporate the new helper.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 54836: Added helpers to allocate / unallocate
Resources.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Dec. 18, 2016, 1:10 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, line 1062
> > <https://reviews.apache.org/r/54836/diff/1/?file=1588580#file1588580line1062>
> >
> > Shall we add a `CHECK` here to make sure this resource was not allocated to any role?
>
> Benjamin Mahler wrote:
> We could, the alternative that I went for was to have callers (the allocator) CHECK this invariant if they care about it, since it seems specific to the way the allocator works.
>
> Guangya Liu wrote:
> I saw that you have updated the comments of `allocate` by allowing existing allocation info be overwitten by a new role, why do we allow this? Which secnario need this operation?
>
> Benjamin Mahler wrote:
> It's not clear if we have a scenario that needs the ability to overwrite, it's just the behavior I would intuit, much like set_role overwrites if there is a role already set.
>
> Guangya Liu wrote:
> Still have one question: For which case shall we need to overwrite the role? Ideally, the role info should be cleared when `recoverResources`, so if there are still roles when `allocate`, we should report an error?
There shouldn't be any use cases in mesos for over-writing a role. Currently there is little checking that unallocated resources are unallocated and allocated resources are allocated, but I'd like to make this less brittle at some point with better types.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54836/#review159564
-----------------------------------------------------------
On Jan. 23, 2017, 1:55 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54836/
> -----------------------------------------------------------
>
> (Updated Jan. 23, 2017, 1:55 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added helpers to allocate / unallocate Resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp c2e6d6680d9687f0bbfb73e84ffb1457072c4412
> include/mesos/v1/resources.hpp 98f025b68c818c384a1174ef9b969f5c0776a7f1
> src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45
> src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68
> src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267
>
> Diff: https://reviews.apache.org/r/54836/diff/
>
>
> Testing
> -------
>
> Updated the existing allocation test to incorporate the new helper.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 54836: Added helpers to allocate / unallocate
Resources.
Posted by Guangya Liu <gy...@gmail.com>.
> On \u5341\u4e8c\u6708 18, 2016, 1:10 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, line 1075
> > <https://reviews.apache.org/r/54836/diff/1/?file=1588580#file1588580line1075>
> >
> > How about put this in the `if` block?
>
> Benjamin Mahler wrote:
> Then this wouldn't clear the allocation?
I mean the following, can this work?
```
if (resource_.resource.has_allocation_info()) {
resource_.resource.clear_allocation_info();
unallocated = true;
}
```
> On \u5341\u4e8c\u6708 18, 2016, 1:10 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, line 1062
> > <https://reviews.apache.org/r/54836/diff/1/?file=1588580#file1588580line1062>
> >
> > Shall we add a `CHECK` here to make sure this resource was not allocated to any role?
>
> Benjamin Mahler wrote:
> We could, the alternative that I went for was to have callers (the allocator) CHECK this invariant if they care about it, since it seems specific to the way the allocator works.
I saw that you have updated the comments of `allocate` by allowing existing allocation info be overwitten by a new role, why do we allow this? Which secnario need this operation?
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54836/#review159564
-----------------------------------------------------------
On \u5341\u4e8c\u6708 18, 2016, 4:36 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54836/
> -----------------------------------------------------------
>
> (Updated \u5341\u4e8c\u6708 18, 2016, 4:36 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Jay Guo, and Guangya Liu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added helpers to allocate / unallocate Resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp c2e6d6680d9687f0bbfb73e84ffb1457072c4412
> include/mesos/v1/resources.hpp 98f025b68c818c384a1174ef9b969f5c0776a7f1
> src/common/resources.cpp 7dbeefc8fc159f28dc1542ad6a761213c65ff226
> src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68
> src/v1/resources.cpp 859f7388035b855bf0cb60d5cbe746e6871bf833
>
> Diff: https://reviews.apache.org/r/54836/diff/
>
>
> Testing
> -------
>
> Updated the existing allocation test to incorporate the new helper.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 54836: Added helpers to allocate / unallocate
Resources.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Dec. 18, 2016, 1:10 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, line 1075
> > <https://reviews.apache.org/r/54836/diff/1/?file=1588580#file1588580line1075>
> >
> > How about put this in the `if` block?
Then this wouldn't clear the allocation?
> On Dec. 18, 2016, 1:10 a.m., Guangya Liu wrote:
> > include/mesos/resources.hpp, line 339
> > <https://reviews.apache.org/r/54836/diff/1/?file=1588578#file1588578line339>
> >
> > What is the use of the return value here, can you please add more comments here for its usage? I think that the `unallocate()` will be called by `recoverResources`, and what is the problem if `unalocate` just return void for such case?
It can return void, which would provide similar semantics to the `Clear()` family of functions in the protobuf api.
I opted instead to return the additional information about whether any changes were needed, so that the caller could do something like:
```
// We don't expect the resources to be already unallocated!
CHECK(resources.unallocate());
```
Like map::erase, or map::insert provides information about this for callers to check (rather than needing an additional explicit check). I clarified the wording here a bit.
> On Dec. 18, 2016, 1:10 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, line 1062
> > <https://reviews.apache.org/r/54836/diff/1/?file=1588580#file1588580line1062>
> >
> > Shall we add a `CHECK` here to make sure this resource was not allocated to any role?
We could, the alternative that I went for was to have callers (the allocator) CHECK this invariant if they care about it, since it seems specific to the way the allocator works.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54836/#review159564
-----------------------------------------------------------
On Dec. 17, 2016, 10:50 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54836/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2016, 10:50 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Jay Guo, and Guangya Liu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added helpers to allocate / unallocate Resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp c2e6d6680d9687f0bbfb73e84ffb1457072c4412
> include/mesos/v1/resources.hpp 98f025b68c818c384a1174ef9b969f5c0776a7f1
> src/common/resources.cpp 16f6de6741b548a8a9fff5b70447b1a312f10efe
> src/tests/resources_tests.cpp c0ff9777ceb1731086179e2966b07c46d035c110
> src/v1/resources.cpp a1adf1b27964bf6a89d97715cff05878d8c6cae9
>
> Diff: https://reviews.apache.org/r/54836/diff/
>
>
> Testing
> -------
>
> Updated the existing allocation test to incorporate the new helper.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 54836: Added helpers to allocate / unallocate
Resources.
Posted by Benjamin Mahler <bm...@apache.org>.
> On Dec. 18, 2016, 1:10 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, line 1075
> > <https://reviews.apache.org/r/54836/diff/1/?file=1588580#file1588580line1075>
> >
> > How about put this in the `if` block?
>
> Benjamin Mahler wrote:
> Then this wouldn't clear the allocation?
>
> Guangya Liu wrote:
> I mean the following, can this work?
>
> ```
> if (resource_.resource.has_allocation_info()) {
> resource_.resource.clear_allocation_info();
> unallocated = true;
> }
> ```
Oh I see, will do.
> On Dec. 18, 2016, 1:10 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, line 1062
> > <https://reviews.apache.org/r/54836/diff/1/?file=1588580#file1588580line1062>
> >
> > Shall we add a `CHECK` here to make sure this resource was not allocated to any role?
>
> Benjamin Mahler wrote:
> We could, the alternative that I went for was to have callers (the allocator) CHECK this invariant if they care about it, since it seems specific to the way the allocator works.
>
> Guangya Liu wrote:
> I saw that you have updated the comments of `allocate` by allowing existing allocation info be overwitten by a new role, why do we allow this? Which secnario need this operation?
It's not clear if we have a scenario that needs the ability to overwrite, it's just the behavior I would intuit, much like set_role overwrites if there is a role already set.
> On Dec. 18, 2016, 1:10 a.m., Guangya Liu wrote:
> > include/mesos/resources.hpp, lines 335-336
> > <https://reviews.apache.org/r/54836/diff/1/?file=1588578#file1588578line335>
> >
> > Can you please elaborate on this? I think that we do not want to allocate a resource again if it was already allocated to some other roles? even with optimistic offer?
Since this is a general Resources class, it shouldn't be coupled to the allocator or any other component logic. So, I've just put in place the behavior that I think is intuitive for a "setter", it just overwrites if already set.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54836/#review159564
-----------------------------------------------------------
On Dec. 18, 2016, 4:36 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54836/
> -----------------------------------------------------------
>
> (Updated Dec. 18, 2016, 4:36 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Jay Guo, and Guangya Liu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added helpers to allocate / unallocate Resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp c2e6d6680d9687f0bbfb73e84ffb1457072c4412
> include/mesos/v1/resources.hpp 98f025b68c818c384a1174ef9b969f5c0776a7f1
> src/common/resources.cpp 7dbeefc8fc159f28dc1542ad6a761213c65ff226
> src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68
> src/v1/resources.cpp 859f7388035b855bf0cb60d5cbe746e6871bf833
>
> Diff: https://reviews.apache.org/r/54836/diff/
>
>
> Testing
> -------
>
> Updated the existing allocation test to incorporate the new helper.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 54836: Added helpers to allocate / unallocate
Resources.
Posted by Guangya Liu <gy...@gmail.com>.
> On \u5341\u4e8c\u6708 18, 2016, 1:10 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, line 1062
> > <https://reviews.apache.org/r/54836/diff/1/?file=1588580#file1588580line1062>
> >
> > Shall we add a `CHECK` here to make sure this resource was not allocated to any role?
>
> Benjamin Mahler wrote:
> We could, the alternative that I went for was to have callers (the allocator) CHECK this invariant if they care about it, since it seems specific to the way the allocator works.
>
> Guangya Liu wrote:
> I saw that you have updated the comments of `allocate` by allowing existing allocation info be overwitten by a new role, why do we allow this? Which secnario need this operation?
>
> Benjamin Mahler wrote:
> It's not clear if we have a scenario that needs the ability to overwrite, it's just the behavior I would intuit, much like set_role overwrites if there is a role already set.
Still have one question: For which case shall we need to overwrite the role? Ideally, the role info should be cleared when `recoverResources`, so if there are still roles when `allocate`, we should report an error?
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54836/#review159564
-----------------------------------------------------------
On \u4e00\u6708 23, 2017, 1:55 a.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54836/
> -----------------------------------------------------------
>
> (Updated \u4e00\u6708 23, 2017, 1:55 a.m.)
>
>
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added helpers to allocate / unallocate Resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp c2e6d6680d9687f0bbfb73e84ffb1457072c4412
> include/mesos/v1/resources.hpp 98f025b68c818c384a1174ef9b969f5c0776a7f1
> src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45
> src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68
> src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267
>
> Diff: https://reviews.apache.org/r/54836/diff/
>
>
> Testing
> -------
>
> Updated the existing allocation test to incorporate the new helper.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 54836: Added helpers to allocate / unallocate
Resources.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54836/#review159564
-----------------------------------------------------------
include/mesos/resources.hpp (lines 335 - 336)
<https://reviews.apache.org/r/54836/#comment230600>
Can you please elaborate on this? I think that we do not want to allocate a resource again if it was already allocated to some other roles? even with optimistic offer?
include/mesos/resources.hpp (line 339)
<https://reviews.apache.org/r/54836/#comment230598>
What is the use of the return value here, can you please add more comments here for its usage? I think that the `unallocate()` will be called by `recoverResources`, and what is the problem if `unalocate` just return void for such case?
src/common/resources.cpp (line 1062)
<https://reviews.apache.org/r/54836/#comment230599>
Shall we add a `CHECK` here to make sure this resource was not allocated to any role?
src/common/resources.cpp (line 1075)
<https://reviews.apache.org/r/54836/#comment230597>
How about put this in the `if` block?
- Guangya Liu
On \u5341\u4e8c\u6708 17, 2016, 10:50 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54836/
> -----------------------------------------------------------
>
> (Updated \u5341\u4e8c\u6708 17, 2016, 10:50 p.m.)
>
>
> Review request for mesos, Benjamin Bannier, Jay Guo, and Guangya Liu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added helpers to allocate / unallocate Resources.
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp c2e6d6680d9687f0bbfb73e84ffb1457072c4412
> include/mesos/v1/resources.hpp 98f025b68c818c384a1174ef9b969f5c0776a7f1
> src/common/resources.cpp 16f6de6741b548a8a9fff5b70447b1a312f10efe
> src/tests/resources_tests.cpp c0ff9777ceb1731086179e2966b07c46d035c110
> src/v1/resources.cpp a1adf1b27964bf6a89d97715cff05878d8c6cae9
>
> Diff: https://reviews.apache.org/r/54836/diff/
>
>
> Testing
> -------
>
> Updated the existing allocation test to incorporate the new helper.
>
>
> Thanks,
>
> Benjamin Mahler
>
>