You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2015/01/21 19:11:26 UTC
Review Request 30130: Killed Resources::Transformation in favor of
using Offer::Operation.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30130/
-----------------------------------------------------------
Review request for mesos, Ben Mahler, Michael Park, and Vinod Kone.
Repository: mesos-git
Description
-------
Killed Resources::Transformation in favor of using Offer::Operation.
This patch is based on and is a replacement for https://reviews.apache.org/r/29744/
Diffs
-----
include/mesos/resources.hpp 7935e7f9bfe66d1900594dcdcb800c4593a3940f
src/common/resources.cpp 214e441fb86aa0c094c28ed5801089051468137b
src/master/allocator.hpp 224569a55eb15d9e7c63f2b571a1f9ae8cd4b6e6
src/master/hierarchical_allocator_process.hpp ccd37b4348c785ece890257f805777acbaf8d58c
src/tests/hierarchical_allocator_tests.cpp 7c051232f2c70691a7789e0f3e94f0e4cba26f18
src/tests/mesos.hpp 591134bab31da0a1cc224e332666d05f320d0f87
src/tests/resources_tests.cpp b7c1ddfda952f2d6b7f82e90cfe7b6b0aafbc36a
src/tests/sorter_tests.cpp 56e5714c2ab97d0ac81d29e1acb1fbec15471489
Diff: https://reviews.apache.org/r/30130/diff/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 30130: Removed Resources::Transformation in favor
of using Offer::Operation.
Posted by Michael Park <mc...@gmail.com>.
> On Jan. 23, 2015, 6:52 a.m., Michael Park wrote:
> >
>
> Michael Park wrote:
> Looks good overall to me, a few minor comments.
> One question: did you not like the generalized logic of `result -= source; result += target;`?
> I'm asking so that I know whether I should generalize it when I go to implement the dynamic reservation parts or not.
>
> Jie Yu wrote:
> What's 'source' and what's 'target'? It's still case by case, so I would rather not to generalize it to make code more localized (thus more readable).
Ok.
> On Jan. 23, 2015, 6:52 a.m., Michael Park wrote:
> > src/common/resources.cpp, lines 627-631
> > <https://reviews.apache.org/r/30130/diff/2/?file=831197#file831197line627>
> >
> > Shouldn't the `validate` function above cover these cases?
>
> Jie Yu wrote:
> The Resources::validate function is only used to verify the integrety of each Resource object (e.g., name='cpu' but has DiskInfo, etc). The validation here is high level checks (i.e., each Resource object is valid, but shoudn't be part of this operation).
Ah ok.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30130/#review69354
-----------------------------------------------------------
On Jan. 23, 2015, 5:16 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30130/
> -----------------------------------------------------------
>
> (Updated Jan. 23, 2015, 5:16 p.m.)
>
>
> Review request for mesos, Ben Mahler, Michael Park, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Removed Resources::Transformation in favor of using Offer::Operation.
>
> This patch is based on and is a replacement for https://reviews.apache.org/r/29744/
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 7935e7f9bfe66d1900594dcdcb800c4593a3940f
> src/common/resources.cpp 214e441fb86aa0c094c28ed5801089051468137b
> src/master/allocator.hpp 224569a55eb15d9e7c63f2b571a1f9ae8cd4b6e6
> src/master/drf_sorter.hpp e0ec58b1095b64f76f6aa4bf1e5d48a77bdf777f
> src/master/drf_sorter.cpp 967c8aa00606445785836c3e3825a6f467746d33
> src/master/hierarchical_allocator_process.hpp ccd37b4348c785ece890257f805777acbaf8d58c
> src/master/master.cpp bda8fda9bc2e52ccc1d75e2541e4604989515e13
> src/master/sorter.hpp 333c2934b1ee01c94bc77b02b607efc438589bd6
> src/tests/hierarchical_allocator_tests.cpp 7c051232f2c70691a7789e0f3e94f0e4cba26f18
> src/tests/mesos.hpp 591134bab31da0a1cc224e332666d05f320d0f87
> src/tests/resources_tests.cpp b7c1ddfda952f2d6b7f82e90cfe7b6b0aafbc36a
> src/tests/sorter_tests.cpp 56e5714c2ab97d0ac81d29e1acb1fbec15471489
>
> Diff: https://reviews.apache.org/r/30130/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 30130: Removed Resources::Transformation in favor
of using Offer::Operation.
Posted by Michael Park <mc...@gmail.com>.
> On Jan. 23, 2015, 6:52 a.m., Michael Park wrote:
> >
Looks good overall to me, a few minor comments.
One question: did you not like the generalized logic of `result -= source; result += target;`?
I'm asking so that I know whether I should generalize it when I go to implement the dynamic reservation parts or not.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30130/#review69354
-----------------------------------------------------------
On Jan. 22, 2015, 11:20 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30130/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2015, 11:20 p.m.)
>
>
> Review request for mesos, Ben Mahler, Michael Park, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Removed Resources::Transformation in favor of using Offer::Operation.
>
> This patch is based on and is a replacement for https://reviews.apache.org/r/29744/
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 7935e7f9bfe66d1900594dcdcb800c4593a3940f
> src/common/resources.cpp 214e441fb86aa0c094c28ed5801089051468137b
> src/master/allocator.hpp 224569a55eb15d9e7c63f2b571a1f9ae8cd4b6e6
> src/master/drf_sorter.hpp e0ec58b1095b64f76f6aa4bf1e5d48a77bdf777f
> src/master/drf_sorter.cpp 967c8aa00606445785836c3e3825a6f467746d33
> src/master/hierarchical_allocator_process.hpp ccd37b4348c785ece890257f805777acbaf8d58c
> src/master/master.cpp bda8fda9bc2e52ccc1d75e2541e4604989515e13
> src/master/sorter.hpp 333c2934b1ee01c94bc77b02b607efc438589bd6
> src/tests/hierarchical_allocator_tests.cpp 7c051232f2c70691a7789e0f3e94f0e4cba26f18
> src/tests/mesos.hpp 591134bab31da0a1cc224e332666d05f320d0f87
> src/tests/resources_tests.cpp b7c1ddfda952f2d6b7f82e90cfe7b6b0aafbc36a
> src/tests/sorter_tests.cpp 56e5714c2ab97d0ac81d29e1acb1fbec15471489
>
> Diff: https://reviews.apache.org/r/30130/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 30130: Removed Resources::Transformation in favor
of using Offer::Operation.
Posted by Jie Yu <yu...@gmail.com>.
> On Jan. 23, 2015, 6:52 a.m., Michael Park wrote:
> >
>
> Michael Park wrote:
> Looks good overall to me, a few minor comments.
> One question: did you not like the generalized logic of `result -= source; result += target;`?
> I'm asking so that I know whether I should generalize it when I go to implement the dynamic reservation parts or not.
What's 'source' and what's 'target'? It's still case by case, so I would rather not to generalize it to make code more localized (thus more readable).
> On Jan. 23, 2015, 6:52 a.m., Michael Park wrote:
> > src/common/resources.cpp, lines 627-631
> > <https://reviews.apache.org/r/30130/diff/2/?file=831197#file831197line627>
> >
> > Shouldn't the `validate` function above cover these cases?
The Resources::validate function is only used to verify the integrety of each Resource object (e.g., name='cpu' but has DiskInfo, etc). The validation here is high level checks (i.e., each Resource object is valid, but shoudn't be part of this operation).
> On Jan. 23, 2015, 6:52 a.m., Michael Park wrote:
> > src/common/resources.cpp, lines 687-692
> > <https://reviews.apache.org/r/30130/diff/2/?file=831197#file831197line687>
> >
> > I think these should be `CHECK`s since these are really post-conditions that we need to assert in our program rather than validation on invalid input.
Good point. I'll do it.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30130/#review69354
-----------------------------------------------------------
On Jan. 22, 2015, 11:20 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30130/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2015, 11:20 p.m.)
>
>
> Review request for mesos, Ben Mahler, Michael Park, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Removed Resources::Transformation in favor of using Offer::Operation.
>
> This patch is based on and is a replacement for https://reviews.apache.org/r/29744/
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 7935e7f9bfe66d1900594dcdcb800c4593a3940f
> src/common/resources.cpp 214e441fb86aa0c094c28ed5801089051468137b
> src/master/allocator.hpp 224569a55eb15d9e7c63f2b571a1f9ae8cd4b6e6
> src/master/drf_sorter.hpp e0ec58b1095b64f76f6aa4bf1e5d48a77bdf777f
> src/master/drf_sorter.cpp 967c8aa00606445785836c3e3825a6f467746d33
> src/master/hierarchical_allocator_process.hpp ccd37b4348c785ece890257f805777acbaf8d58c
> src/master/master.cpp bda8fda9bc2e52ccc1d75e2541e4604989515e13
> src/master/sorter.hpp 333c2934b1ee01c94bc77b02b607efc438589bd6
> src/tests/hierarchical_allocator_tests.cpp 7c051232f2c70691a7789e0f3e94f0e4cba26f18
> src/tests/mesos.hpp 591134bab31da0a1cc224e332666d05f320d0f87
> src/tests/resources_tests.cpp b7c1ddfda952f2d6b7f82e90cfe7b6b0aafbc36a
> src/tests/sorter_tests.cpp 56e5714c2ab97d0ac81d29e1acb1fbec15471489
>
> Diff: https://reviews.apache.org/r/30130/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 30130: Removed Resources::Transformation in favor
of using Offer::Operation.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30130/#review69354
-----------------------------------------------------------
src/common/resources.cpp
<https://reviews.apache.org/r/30130/#comment114032>
Shouldn't the `validate` function above cover these cases?
src/common/resources.cpp
<https://reviews.apache.org/r/30130/#comment114033>
I think these should be `CHECK`s since these are really post-conditions that we need to assert in our program rather than validation on invalid input.
- Michael Park
On Jan. 22, 2015, 11:20 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30130/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2015, 11:20 p.m.)
>
>
> Review request for mesos, Ben Mahler, Michael Park, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Removed Resources::Transformation in favor of using Offer::Operation.
>
> This patch is based on and is a replacement for https://reviews.apache.org/r/29744/
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 7935e7f9bfe66d1900594dcdcb800c4593a3940f
> src/common/resources.cpp 214e441fb86aa0c094c28ed5801089051468137b
> src/master/allocator.hpp 224569a55eb15d9e7c63f2b571a1f9ae8cd4b6e6
> src/master/drf_sorter.hpp e0ec58b1095b64f76f6aa4bf1e5d48a77bdf777f
> src/master/drf_sorter.cpp 967c8aa00606445785836c3e3825a6f467746d33
> src/master/hierarchical_allocator_process.hpp ccd37b4348c785ece890257f805777acbaf8d58c
> src/master/master.cpp bda8fda9bc2e52ccc1d75e2541e4604989515e13
> src/master/sorter.hpp 333c2934b1ee01c94bc77b02b607efc438589bd6
> src/tests/hierarchical_allocator_tests.cpp 7c051232f2c70691a7789e0f3e94f0e4cba26f18
> src/tests/mesos.hpp 591134bab31da0a1cc224e332666d05f320d0f87
> src/tests/resources_tests.cpp b7c1ddfda952f2d6b7f82e90cfe7b6b0aafbc36a
> src/tests/sorter_tests.cpp 56e5714c2ab97d0ac81d29e1acb1fbec15471489
>
> Diff: https://reviews.apache.org/r/30130/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 30130: Removed Resources::Transformation in favor
of using Offer::Operation.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30130/#review69643
-----------------------------------------------------------
Ship it!
src/master/sorter.hpp
<https://reviews.apache.org/r/30130/#comment114380>
Can we add an example in this comment?
// ... additional metadata (e.g. volumes).
src/tests/resources_tests.cpp
<https://reviews.apache.org/r/30130/#comment114381>
s/Transformation/Operation/ ?
- Ben Mahler
On Jan. 23, 2015, 5:16 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30130/
> -----------------------------------------------------------
>
> (Updated Jan. 23, 2015, 5:16 p.m.)
>
>
> Review request for mesos, Ben Mahler, Michael Park, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Removed Resources::Transformation in favor of using Offer::Operation.
>
> This patch is based on and is a replacement for https://reviews.apache.org/r/29744/
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 7935e7f9bfe66d1900594dcdcb800c4593a3940f
> src/common/resources.cpp 214e441fb86aa0c094c28ed5801089051468137b
> src/master/allocator.hpp 224569a55eb15d9e7c63f2b571a1f9ae8cd4b6e6
> src/master/drf_sorter.hpp e0ec58b1095b64f76f6aa4bf1e5d48a77bdf777f
> src/master/drf_sorter.cpp 967c8aa00606445785836c3e3825a6f467746d33
> src/master/hierarchical_allocator_process.hpp ccd37b4348c785ece890257f805777acbaf8d58c
> src/master/master.cpp bda8fda9bc2e52ccc1d75e2541e4604989515e13
> src/master/sorter.hpp 333c2934b1ee01c94bc77b02b607efc438589bd6
> src/tests/hierarchical_allocator_tests.cpp 7c051232f2c70691a7789e0f3e94f0e4cba26f18
> src/tests/mesos.hpp 591134bab31da0a1cc224e332666d05f320d0f87
> src/tests/resources_tests.cpp b7c1ddfda952f2d6b7f82e90cfe7b6b0aafbc36a
> src/tests/sorter_tests.cpp 56e5714c2ab97d0ac81d29e1acb1fbec15471489
>
> Diff: https://reviews.apache.org/r/30130/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 30130: Removed Resources::Transformation in favor
of using Offer::Operation.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30130/
-----------------------------------------------------------
(Updated Jan. 23, 2015, 5:16 p.m.)
Review request for mesos, Ben Mahler, Michael Park, and Vinod Kone.
Changes
-------
Mpark's comments.
Repository: mesos-git
Description
-------
Removed Resources::Transformation in favor of using Offer::Operation.
This patch is based on and is a replacement for https://reviews.apache.org/r/29744/
Diffs (updated)
-----
include/mesos/resources.hpp 7935e7f9bfe66d1900594dcdcb800c4593a3940f
src/common/resources.cpp 214e441fb86aa0c094c28ed5801089051468137b
src/master/allocator.hpp 224569a55eb15d9e7c63f2b571a1f9ae8cd4b6e6
src/master/drf_sorter.hpp e0ec58b1095b64f76f6aa4bf1e5d48a77bdf777f
src/master/drf_sorter.cpp 967c8aa00606445785836c3e3825a6f467746d33
src/master/hierarchical_allocator_process.hpp ccd37b4348c785ece890257f805777acbaf8d58c
src/master/master.cpp bda8fda9bc2e52ccc1d75e2541e4604989515e13
src/master/sorter.hpp 333c2934b1ee01c94bc77b02b607efc438589bd6
src/tests/hierarchical_allocator_tests.cpp 7c051232f2c70691a7789e0f3e94f0e4cba26f18
src/tests/mesos.hpp 591134bab31da0a1cc224e332666d05f320d0f87
src/tests/resources_tests.cpp b7c1ddfda952f2d6b7f82e90cfe7b6b0aafbc36a
src/tests/sorter_tests.cpp 56e5714c2ab97d0ac81d29e1acb1fbec15471489
Diff: https://reviews.apache.org/r/30130/diff/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 30130: Removed Resources::Transformation in favor
of using Offer::Operation.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30130/
-----------------------------------------------------------
(Updated Jan. 22, 2015, 11:20 p.m.)
Review request for mesos, Ben Mahler, Michael Park, and Vinod Kone.
Changes
-------
BenM's comments.
Summary (updated)
-----------------
Removed Resources::Transformation in favor of using Offer::Operation.
Repository: mesos-git
Description (updated)
-------
Removed Resources::Transformation in favor of using Offer::Operation.
This patch is based on and is a replacement for https://reviews.apache.org/r/29744/
Diffs (updated)
-----
include/mesos/resources.hpp 7935e7f9bfe66d1900594dcdcb800c4593a3940f
src/common/resources.cpp 214e441fb86aa0c094c28ed5801089051468137b
src/master/allocator.hpp 224569a55eb15d9e7c63f2b571a1f9ae8cd4b6e6
src/master/drf_sorter.hpp e0ec58b1095b64f76f6aa4bf1e5d48a77bdf777f
src/master/drf_sorter.cpp 967c8aa00606445785836c3e3825a6f467746d33
src/master/hierarchical_allocator_process.hpp ccd37b4348c785ece890257f805777acbaf8d58c
src/master/master.cpp bda8fda9bc2e52ccc1d75e2541e4604989515e13
src/master/sorter.hpp 333c2934b1ee01c94bc77b02b607efc438589bd6
src/tests/hierarchical_allocator_tests.cpp 7c051232f2c70691a7789e0f3e94f0e4cba26f18
src/tests/mesos.hpp 591134bab31da0a1cc224e332666d05f320d0f87
src/tests/resources_tests.cpp b7c1ddfda952f2d6b7f82e90cfe7b6b0aafbc36a
src/tests/sorter_tests.cpp 56e5714c2ab97d0ac81d29e1acb1fbec15471489
Diff: https://reviews.apache.org/r/30130/diff/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 30130: Killed Resources::Transformation in favor
of using Offer::Operation.
Posted by Jie Yu <yu...@gmail.com>.
> On Jan. 22, 2015, 12:43 a.m., Ben Mahler wrote:
> > include/mesos/resources.hpp, line 157
> > <https://reviews.apache.org/r/30130/diff/1/?file=828560#file828560line157>
> >
> > How are you going to use this one? It seems like we will normally have a `RepeatedPtrField<Offer::Operation>`, do you want that instead?
> >
> > Or do you want a templatized version?
> >
> > ```
> > template <typename Iterable>
> > Try<Resources> apply(const Iterable<Offer::Operation>& operations) const;
> > ```
I go with the templatized version.
> On Jan. 22, 2015, 12:43 a.m., Ben Mahler wrote:
> > src/common/resources.cpp, lines 629-631
> > <https://reviews.apache.org/r/30130/diff/1/?file=828561#file828561line629>
> >
> > How about the following to be a bit more explicit and consistent:
> >
> > ```
> > if (!volume.has_disk()) {
> > return Error("Invalid CREATE Operation: Expecting 'disk' to be present");
> > } else if (!volume.disk().has_persistence)) {
> > return Error("Invalid CREATE Operation: Expecting 'persistence' to be present");
> > }
> > ```
> >
> > Could you use this pattern througout this method for all Error returns?
> >
> > ```
> > "Invalid X Operation: <reason>"
> > ```
> >
> > Some of the existing errors don't include the operation type.
Done.
> On Jan. 22, 2015, 12:43 a.m., Ben Mahler wrote:
> > src/common/resources.cpp, lines 661-663
> > <https://reviews.apache.org/r/30130/diff/1/?file=828561#file828561line661>
> >
> > Ditto here for splitting these two cases apart.
Done.
> On Jan. 22, 2015, 12:43 a.m., Ben Mahler wrote:
> > src/master/hierarchical_allocator_process.hpp, lines 603-604
> > <https://reviews.apache.org/r/30130/diff/1/?file=828563#file828563line603>
> >
> > What happened here? Mind leaving as is to make the diff smaller?
Reverted.
> On Jan. 22, 2015, 12:43 a.m., Ben Mahler wrote:
> > src/master/allocator.hpp, line 202
> > <https://reviews.apache.org/r/30130/diff/1/?file=828562#file828562line202>
> >
> > How about 'updateAllocation' now that we avoid using "transformation"?
I am renamed transform to update in sorter.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30130/#review69089
-----------------------------------------------------------
On Jan. 21, 2015, 6:11 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30130/
> -----------------------------------------------------------
>
> (Updated Jan. 21, 2015, 6:11 p.m.)
>
>
> Review request for mesos, Ben Mahler, Michael Park, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Killed Resources::Transformation in favor of using Offer::Operation.
>
> This patch is based on and is a replacement for https://reviews.apache.org/r/29744/
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 7935e7f9bfe66d1900594dcdcb800c4593a3940f
> src/common/resources.cpp 214e441fb86aa0c094c28ed5801089051468137b
> src/master/allocator.hpp 224569a55eb15d9e7c63f2b571a1f9ae8cd4b6e6
> src/master/hierarchical_allocator_process.hpp ccd37b4348c785ece890257f805777acbaf8d58c
> src/tests/hierarchical_allocator_tests.cpp 7c051232f2c70691a7789e0f3e94f0e4cba26f18
> src/tests/mesos.hpp 591134bab31da0a1cc224e332666d05f320d0f87
> src/tests/resources_tests.cpp b7c1ddfda952f2d6b7f82e90cfe7b6b0aafbc36a
> src/tests/sorter_tests.cpp 56e5714c2ab97d0ac81d29e1acb1fbec15471489
>
> Diff: https://reviews.apache.org/r/30130/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 30130: Killed Resources::Transformation in favor
of using Offer::Operation.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30130/#review69089
-----------------------------------------------------------
What does it mean to kill code? ;) How about s/Killed/Removed/ in the title and description?
Looks good, just some minor comments and we should be good to go.
include/mesos/resources.hpp
<https://reviews.apache.org/r/30130/#comment113694>
How are you going to use this one? It seems like we will normally have a `RepeatedPtrField<Offer::Operation>`, do you want that instead?
Or do you want a templatized version?
```
template <typename Iterable>
Try<Resources> apply(const Iterable<Offer::Operation>& operations) const;
```
src/common/resources.cpp
<https://reviews.apache.org/r/30130/#comment113697>
How about the following to be a bit more explicit and consistent:
```
if (!volume.has_disk()) {
return Error("Invalid CREATE Operation: Expecting 'disk' to be present");
} else if (!volume.disk().has_persistence)) {
return Error("Invalid CREATE Operation: Expecting 'persistence' to be present");
}
```
Could you use this pattern througout this method for all Error returns?
```
"Invalid X Operation: <reason>"
```
Some of the existing errors don't include the operation type.
src/common/resources.cpp
<https://reviews.apache.org/r/30130/#comment113703>
Ditto here for splitting these two cases apart.
src/common/resources.cpp
<https://reviews.apache.org/r/30130/#comment113705>
Shouldn't this be printing the number of the type?
As is it will just print the empty string if someone sets this incorrectly.
Or does protobuf prevent an invalid enum value..?
src/common/resources.cpp
<https://reviews.apache.org/r/30130/#comment113706>
"check"
src/master/allocator.hpp
<https://reviews.apache.org/r/30130/#comment113688>
How about 'updateAllocation' now that we avoid using "transformation"?
src/master/hierarchical_allocator_process.hpp
<https://reviews.apache.org/r/30130/#comment113686>
What happened here? Mind leaving as is to make the diff smaller?
- Ben Mahler
On Jan. 21, 2015, 6:11 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30130/
> -----------------------------------------------------------
>
> (Updated Jan. 21, 2015, 6:11 p.m.)
>
>
> Review request for mesos, Ben Mahler, Michael Park, and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Killed Resources::Transformation in favor of using Offer::Operation.
>
> This patch is based on and is a replacement for https://reviews.apache.org/r/29744/
>
>
> Diffs
> -----
>
> include/mesos/resources.hpp 7935e7f9bfe66d1900594dcdcb800c4593a3940f
> src/common/resources.cpp 214e441fb86aa0c094c28ed5801089051468137b
> src/master/allocator.hpp 224569a55eb15d9e7c63f2b571a1f9ae8cd4b6e6
> src/master/hierarchical_allocator_process.hpp ccd37b4348c785ece890257f805777acbaf8d58c
> src/tests/hierarchical_allocator_tests.cpp 7c051232f2c70691a7789e0f3e94f0e4cba26f18
> src/tests/mesos.hpp 591134bab31da0a1cc224e332666d05f320d0f87
> src/tests/resources_tests.cpp b7c1ddfda952f2d6b7f82e90cfe7b6b0aafbc36a
> src/tests/sorter_tests.cpp 56e5714c2ab97d0ac81d29e1acb1fbec15471489
>
> Diff: https://reviews.apache.org/r/30130/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>