You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Neil Conway <ne...@gmail.com> on 2016/02/03 18:51:41 UTC
Re: Review Request 42754: Added support for labels to resource
reservations.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42754/
-----------------------------------------------------------
(Updated Feb. 3, 2016, 5:51 p.m.)
Review request for mesos and Michael Park.
Changes
-------
Rebase.
Bugs: MESOS-4479
https://issues.apache.org/jira/browse/MESOS-4479
Repository: mesos
Description
-------
Labels are free-form key-value pairs that can be used to associate
metadata with reserved resources.
Diffs (updated)
-----
include/mesos/mesos.proto 194750e92020753e60154083a47bdc3398d31466
include/mesos/type_utils.hpp 16f9cda83edd5064527b89e94392a7a111154411
include/mesos/v1/mesos.hpp b294f022345dc892c0581622c5b389efbde911a9
include/mesos/v1/mesos.proto 1102bbc92f46f97c1915c03a71c7cf829003e0ed
src/common/resources.cpp 588a279c3cdebbeb58047bfbff5e78a42f53fc13
src/common/type_utils.cpp 42e3061818c49c6b62d4fee2479db078dfe0fc89
src/tests/mesos.hpp c2bae4767ee7372c796bfad44ed1e86db7dd3488
src/tests/reservation_endpoints_tests.cpp 35c093567b07a11ca6eee85e2ff91894a460a7af
src/tests/resources_tests.cpp 4b25e82c13e4f46c73803f773db90f269c09c48a
src/v1/mesos.cpp 21b14cf7050270facab1ba2b844c3fde664665a4
src/v1/resources.cpp be4a5d153e9313cb71a6e85d1ed25a358537f2b7
Diff: https://reviews.apache.org/r/42754/diff/
Testing
-------
make check
Thanks,
Neil Conway
Re: Review Request 42754: Added support for labels to resource
reservations.
Posted by Neil Conway <ne...@gmail.com>.
> On Feb. 4, 2016, 1:32 a.m., Michael Park wrote:
> > src/tests/resources_tests.cpp, line 35
> > <https://reviews.apache.org/r/42754/diff/3/?file=1231634#file1231634line35>
> >
> > Should come after `std` using declarations.
Happy to change this, but I notice that a few other places don't do this consistently (e.g., `slave_tests.cpp`, `sorter_tests.cpp`, `slave_recovery_tests.cpp`, `role_tests.cpp`). Do we have a canonical rule for how to order `using` declarations?
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42754/#review117730
-----------------------------------------------------------
On Feb. 3, 2016, 11:32 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42754/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2016, 11:32 p.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Bugs: MESOS-4479
> https://issues.apache.org/jira/browse/MESOS-4479
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Labels are free-form key-value pairs that can be used to associate
> metadata with reserved resources.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 194750e92020753e60154083a47bdc3398d31466
> include/mesos/type_utils.hpp 16f9cda83edd5064527b89e94392a7a111154411
> include/mesos/v1/mesos.hpp b294f022345dc892c0581622c5b389efbde911a9
> include/mesos/v1/mesos.proto 1102bbc92f46f97c1915c03a71c7cf829003e0ed
> src/common/resources.cpp 588a279c3cdebbeb58047bfbff5e78a42f53fc13
> src/common/type_utils.cpp 42e3061818c49c6b62d4fee2479db078dfe0fc89
> src/tests/mesos.hpp c2bae4767ee7372c796bfad44ed1e86db7dd3488
> src/tests/reservation_endpoints_tests.cpp 35c093567b07a11ca6eee85e2ff91894a460a7af
> src/tests/resources_tests.cpp 4b25e82c13e4f46c73803f773db90f269c09c48a
> src/v1/mesos.cpp 21b14cf7050270facab1ba2b844c3fde664665a4
> src/v1/resources.cpp be4a5d153e9313cb71a6e85d1ed25a358537f2b7
>
> Diff: https://reviews.apache.org/r/42754/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 42754: Added support for labels to resource
reservations.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42754/#review117730
-----------------------------------------------------------
src/tests/reservation_endpoints_tests.cpp (lines 752 - 757)
<https://reviews.apache.org/r/42754/#comment179012>
Since the unreserve operation would rescind resources as necessary, I don't think we need to do this. Is this not the case?
src/tests/resources_tests.cpp (line 35)
<https://reviews.apache.org/r/42754/#comment179013>
Should come after `std` using declarations.
src/tests/resources_tests.cpp (line 1650)
<https://reviews.apache.org/r/42754/#comment179015>
`s/AdditionDynamicallyReserved/AdditionDynamicallyReservedWithoutLabels/`
src/tests/resources_tests.cpp (line 1666)
<https://reviews.apache.org/r/42754/#comment179009>
`s/AdditionDynamicallyReservedWithLabels/AdditionDynamicallyReservedWithSameLabels/`?
src/tests/resources_tests.cpp (line 1708)
<https://reviews.apache.org/r/42754/#comment179016>
Weird that we didn't have `SubtractionStaticallyReserved` and `SubtractionDynamicallyReserved` before. Can we take this opportunity to sync `Addition` and `Subtraction`? or do you think there's a good reason to keep them conflated in this case?
As written, it looks like we're expecting the correct portions (statically reserved from `r1` and dynamically reserved from `r2`) of `total` to be subtracted by `r7` and `r8`. I'd ok with keeping this pattern, and if we were to do that, I think we should add the labeled states into the `total` and expect the correct portions of them to be subtracted.
What do you think?
- Michael Park
On Feb. 3, 2016, 11:32 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42754/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2016, 11:32 p.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Bugs: MESOS-4479
> https://issues.apache.org/jira/browse/MESOS-4479
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Labels are free-form key-value pairs that can be used to associate
> metadata with reserved resources.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 194750e92020753e60154083a47bdc3398d31466
> include/mesos/type_utils.hpp 16f9cda83edd5064527b89e94392a7a111154411
> include/mesos/v1/mesos.hpp b294f022345dc892c0581622c5b389efbde911a9
> include/mesos/v1/mesos.proto 1102bbc92f46f97c1915c03a71c7cf829003e0ed
> src/common/resources.cpp 588a279c3cdebbeb58047bfbff5e78a42f53fc13
> src/common/type_utils.cpp 42e3061818c49c6b62d4fee2479db078dfe0fc89
> src/tests/mesos.hpp c2bae4767ee7372c796bfad44ed1e86db7dd3488
> src/tests/reservation_endpoints_tests.cpp 35c093567b07a11ca6eee85e2ff91894a460a7af
> src/tests/resources_tests.cpp 4b25e82c13e4f46c73803f773db90f269c09c48a
> src/v1/mesos.cpp 21b14cf7050270facab1ba2b844c3fde664665a4
> src/v1/resources.cpp be4a5d153e9313cb71a6e85d1ed25a358537f2b7
>
> Diff: https://reviews.apache.org/r/42754/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 42754: Added support for labels to resource
reservations.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42754/
-----------------------------------------------------------
(Updated Feb. 10, 2016, 6:59 p.m.)
Review request for mesos and Michael Park.
Changes
-------
Address (most) review comments.
Bugs: MESOS-4479
https://issues.apache.org/jira/browse/MESOS-4479
Repository: mesos
Description
-------
Labels are free-form key-value pairs that can be used to associate
metadata with reserved resources.
Diffs (updated)
-----
include/mesos/mesos.proto 5d1a20b4ff647f43701a353472b56a4b74b6bbc3
include/mesos/type_utils.hpp 16f9cda83edd5064527b89e94392a7a111154411
include/mesos/v1/mesos.hpp b294f022345dc892c0581622c5b389efbde911a9
include/mesos/v1/mesos.proto 415a20d9f3385e9627635dd89d78c58d03e3c116
src/common/resources.cpp 588a279c3cdebbeb58047bfbff5e78a42f53fc13
src/common/type_utils.cpp 42e3061818c49c6b62d4fee2479db078dfe0fc89
src/tests/mesos.hpp c0997dbacf8753149caba0a8b2406afc35a4845f
src/tests/reservation_endpoints_tests.cpp 0a8c479b6684a4497f53c81c198bfcf8e879aa54
src/tests/resources_tests.cpp a190cc3900be72031e1c13e5843245f0f82dbf99
src/v1/mesos.cpp 21b14cf7050270facab1ba2b844c3fde664665a4
src/v1/resources.cpp be4a5d153e9313cb71a6e85d1ed25a358537f2b7
Diff: https://reviews.apache.org/r/42754/diff/
Testing
-------
make check
Thanks,
Neil Conway
Re: Review Request 42754: Added support for labels to resource
reservations.
Posted by Michael Park <mp...@apache.org>.
> On Feb. 10, 2016, 12:19 a.m., Michael Park wrote:
> > include/mesos/type_utils.hpp, line 71
> > <https://reviews.apache.org/r/42754/diff/6/?file=1235807#file1235807line71>
> >
> > We should keep the 2 newlines here. There are other parts that need 2 lines also, for example between `bool operator!=(const TaskStatus& left, const TaskStatus& right);` and `inline bool operator==(const ContainerID& left, const ContainerID& right)`.
>
> Neil Conway wrote:
> What is the rationale for keeping the two lines? It doesn't seem consistent with the spacing in the rest of the file.
>
> I fixed the second whitespace issue you mentioned.
Synced with Neil on this. I don't quite know the rationale for keeping __2__ newlines, but we use 2 newlines when we need "spacing" in the namespace scope, whereas we use 1 newline when we need "spacing" in a class scope. We need spacing when either of 2 declarations span multiple lines.
e.g.
```cpp
namespace X {
void F();
void G();
} // namespace X {
```
```cpp
namespace X {
void F(
SomeReallyReallyLongType someReallyReallyLongName,
SomeReallyReallyLongType someReallyReallyLongName);
void G();
} // namespace X {
```
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42754/#review118506
-----------------------------------------------------------
On Feb. 10, 2016, 6:59 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42754/
> -----------------------------------------------------------
>
> (Updated Feb. 10, 2016, 6:59 p.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Bugs: MESOS-4479
> https://issues.apache.org/jira/browse/MESOS-4479
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Labels are free-form key-value pairs that can be used to associate
> metadata with reserved resources.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 5d1a20b4ff647f43701a353472b56a4b74b6bbc3
> include/mesos/type_utils.hpp 16f9cda83edd5064527b89e94392a7a111154411
> include/mesos/v1/mesos.hpp b294f022345dc892c0581622c5b389efbde911a9
> include/mesos/v1/mesos.proto 415a20d9f3385e9627635dd89d78c58d03e3c116
> src/common/resources.cpp 588a279c3cdebbeb58047bfbff5e78a42f53fc13
> src/common/type_utils.cpp 42e3061818c49c6b62d4fee2479db078dfe0fc89
> src/tests/mesos.hpp c0997dbacf8753149caba0a8b2406afc35a4845f
> src/tests/reservation_endpoints_tests.cpp 0a8c479b6684a4497f53c81c198bfcf8e879aa54
> src/tests/resources_tests.cpp a190cc3900be72031e1c13e5843245f0f82dbf99
> src/v1/mesos.cpp 21b14cf7050270facab1ba2b844c3fde664665a4
> src/v1/resources.cpp be4a5d153e9313cb71a6e85d1ed25a358537f2b7
>
> Diff: https://reviews.apache.org/r/42754/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 42754: Added support for labels to resource
reservations.
Posted by Neil Conway <ne...@gmail.com>.
> On Feb. 10, 2016, 12:19 a.m., Michael Park wrote:
> > include/mesos/type_utils.hpp, line 71
> > <https://reviews.apache.org/r/42754/diff/6/?file=1235807#file1235807line71>
> >
> > We should keep the 2 newlines here. There are other parts that need 2 lines also, for example between `bool operator!=(const TaskStatus& left, const TaskStatus& right);` and `inline bool operator==(const ContainerID& left, const ContainerID& right)`.
What is the rationale for keeping the two lines? It doesn't seem consistent with the spacing in the rest of the file.
I fixed the second whitespace issue you mentioned.
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42754/#review118506
-----------------------------------------------------------
On Feb. 5, 2016, 2:19 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42754/
> -----------------------------------------------------------
>
> (Updated Feb. 5, 2016, 2:19 a.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Bugs: MESOS-4479
> https://issues.apache.org/jira/browse/MESOS-4479
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Labels are free-form key-value pairs that can be used to associate
> metadata with reserved resources.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 5d1a20b4ff647f43701a353472b56a4b74b6bbc3
> include/mesos/type_utils.hpp 16f9cda83edd5064527b89e94392a7a111154411
> include/mesos/v1/mesos.hpp b294f022345dc892c0581622c5b389efbde911a9
> include/mesos/v1/mesos.proto 415a20d9f3385e9627635dd89d78c58d03e3c116
> src/common/resources.cpp 588a279c3cdebbeb58047bfbff5e78a42f53fc13
> src/common/type_utils.cpp 42e3061818c49c6b62d4fee2479db078dfe0fc89
> src/tests/mesos.hpp e07d8aa6f1e507a91ce70763aafa134bdd9a7ec8
> src/tests/reservation_endpoints_tests.cpp 35c093567b07a11ca6eee85e2ff91894a460a7af
> src/tests/resources_tests.cpp 4b25e82c13e4f46c73803f773db90f269c09c48a
> src/v1/mesos.cpp 21b14cf7050270facab1ba2b844c3fde664665a4
> src/v1/resources.cpp be4a5d153e9313cb71a6e85d1ed25a358537f2b7
>
> Diff: https://reviews.apache.org/r/42754/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 42754: Added support for labels to resource
reservations.
Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42754/#review118506
-----------------------------------------------------------
Fix it, then Ship it!
include/mesos/type_utils.hpp (line 71)
<https://reviews.apache.org/r/42754/#comment179765>
We should keep the 2 newlines here. There are other parts that need 2 lines also, for example between `bool operator!=(const TaskStatus& left, const TaskStatus& right);` and `inline bool operator==(const ContainerID& left, const ContainerID& right)`.
src/common/resources.cpp (lines 1494 - 1500)
<https://reviews.apache.org/r/42754/#comment179768>
```
stream << label.key();
if (label.has_value()) {
stream << ": " << label.value();
}
if (i + 1 < labels.labels().size()) {
stream << ", ";
}
```
Same in the v1 version.
src/tests/mesos.hpp (lines 494 - 495)
<https://reviews.apache.org/r/42754/#comment179766>
Newline here
src/tests/reservation_endpoints_tests.cpp (line 743)
<https://reviews.apache.org/r/42754/#comment179759>
`s/offerResources/offeredResources/`? here and below
- Michael Park
On Feb. 5, 2016, 2:19 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42754/
> -----------------------------------------------------------
>
> (Updated Feb. 5, 2016, 2:19 a.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Bugs: MESOS-4479
> https://issues.apache.org/jira/browse/MESOS-4479
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Labels are free-form key-value pairs that can be used to associate
> metadata with reserved resources.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 5d1a20b4ff647f43701a353472b56a4b74b6bbc3
> include/mesos/type_utils.hpp 16f9cda83edd5064527b89e94392a7a111154411
> include/mesos/v1/mesos.hpp b294f022345dc892c0581622c5b389efbde911a9
> include/mesos/v1/mesos.proto 415a20d9f3385e9627635dd89d78c58d03e3c116
> src/common/resources.cpp 588a279c3cdebbeb58047bfbff5e78a42f53fc13
> src/common/type_utils.cpp 42e3061818c49c6b62d4fee2479db078dfe0fc89
> src/tests/mesos.hpp e07d8aa6f1e507a91ce70763aafa134bdd9a7ec8
> src/tests/reservation_endpoints_tests.cpp 35c093567b07a11ca6eee85e2ff91894a460a7af
> src/tests/resources_tests.cpp 4b25e82c13e4f46c73803f773db90f269c09c48a
> src/v1/mesos.cpp 21b14cf7050270facab1ba2b844c3fde664665a4
> src/v1/resources.cpp be4a5d153e9313cb71a6e85d1ed25a358537f2b7
>
> Diff: https://reviews.apache.org/r/42754/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 42754: Added support for labels to resource
reservations.
Posted by Michael Park <mp...@apache.org>.
> On Feb. 10, 2016, 8:18 a.m., Guangya Liu wrote:
> > include/mesos/v1/mesos.hpp, line 64
> > <https://reviews.apache.org/r/42754/diff/6/?file=1235808#file1235808line64>
> >
> > two lines here
I'll follow up with a trivial formatting patch after this.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42754/#review118612
-----------------------------------------------------------
On Feb. 10, 2016, 6:59 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42754/
> -----------------------------------------------------------
>
> (Updated Feb. 10, 2016, 6:59 p.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Bugs: MESOS-4479
> https://issues.apache.org/jira/browse/MESOS-4479
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Labels are free-form key-value pairs that can be used to associate
> metadata with reserved resources.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 5d1a20b4ff647f43701a353472b56a4b74b6bbc3
> include/mesos/type_utils.hpp 16f9cda83edd5064527b89e94392a7a111154411
> include/mesos/v1/mesos.hpp b294f022345dc892c0581622c5b389efbde911a9
> include/mesos/v1/mesos.proto 415a20d9f3385e9627635dd89d78c58d03e3c116
> src/common/resources.cpp 588a279c3cdebbeb58047bfbff5e78a42f53fc13
> src/common/type_utils.cpp 42e3061818c49c6b62d4fee2479db078dfe0fc89
> src/tests/mesos.hpp c0997dbacf8753149caba0a8b2406afc35a4845f
> src/tests/reservation_endpoints_tests.cpp 0a8c479b6684a4497f53c81c198bfcf8e879aa54
> src/tests/resources_tests.cpp a190cc3900be72031e1c13e5843245f0f82dbf99
> src/v1/mesos.cpp 21b14cf7050270facab1ba2b844c3fde664665a4
> src/v1/resources.cpp be4a5d153e9313cb71a6e85d1ed25a358537f2b7
>
> Diff: https://reviews.apache.org/r/42754/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 42754: Added support for labels to resource
reservations.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42754/#review118612
-----------------------------------------------------------
include/mesos/v1/mesos.hpp (line 64)
<https://reviews.apache.org/r/42754/#comment179894>
two lines here
include/mesos/v1/mesos.hpp (line 67)
<https://reviews.apache.org/r/42754/#comment179895>
it would be great if you can also help add a new line here
- Guangya Liu
On 二月 5, 2016, 2:19 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42754/
> -----------------------------------------------------------
>
> (Updated 二月 5, 2016, 2:19 a.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Bugs: MESOS-4479
> https://issues.apache.org/jira/browse/MESOS-4479
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Labels are free-form key-value pairs that can be used to associate
> metadata with reserved resources.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 5d1a20b4ff647f43701a353472b56a4b74b6bbc3
> include/mesos/type_utils.hpp 16f9cda83edd5064527b89e94392a7a111154411
> include/mesos/v1/mesos.hpp b294f022345dc892c0581622c5b389efbde911a9
> include/mesos/v1/mesos.proto 415a20d9f3385e9627635dd89d78c58d03e3c116
> src/common/resources.cpp 588a279c3cdebbeb58047bfbff5e78a42f53fc13
> src/common/type_utils.cpp 42e3061818c49c6b62d4fee2479db078dfe0fc89
> src/tests/mesos.hpp e07d8aa6f1e507a91ce70763aafa134bdd9a7ec8
> src/tests/reservation_endpoints_tests.cpp 35c093567b07a11ca6eee85e2ff91894a460a7af
> src/tests/resources_tests.cpp 4b25e82c13e4f46c73803f773db90f269c09c48a
> src/v1/mesos.cpp 21b14cf7050270facab1ba2b844c3fde664665a4
> src/v1/resources.cpp be4a5d153e9313cb71a6e85d1ed25a358537f2b7
>
> Diff: https://reviews.apache.org/r/42754/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 42754: Added support for labels to resource
reservations.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42754/#review118700
-----------------------------------------------------------
include/mesos/type_utils.hpp (line 71)
<https://reviews.apache.org/r/42754/#comment180004>
What is the rationale for keeping 2 newlines?
I fixed the newline issue below.
- Neil Conway
On Feb. 5, 2016, 2:19 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42754/
> -----------------------------------------------------------
>
> (Updated Feb. 5, 2016, 2:19 a.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Bugs: MESOS-4479
> https://issues.apache.org/jira/browse/MESOS-4479
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Labels are free-form key-value pairs that can be used to associate
> metadata with reserved resources.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 5d1a20b4ff647f43701a353472b56a4b74b6bbc3
> include/mesos/type_utils.hpp 16f9cda83edd5064527b89e94392a7a111154411
> include/mesos/v1/mesos.hpp b294f022345dc892c0581622c5b389efbde911a9
> include/mesos/v1/mesos.proto 415a20d9f3385e9627635dd89d78c58d03e3c116
> src/common/resources.cpp 588a279c3cdebbeb58047bfbff5e78a42f53fc13
> src/common/type_utils.cpp 42e3061818c49c6b62d4fee2479db078dfe0fc89
> src/tests/mesos.hpp e07d8aa6f1e507a91ce70763aafa134bdd9a7ec8
> src/tests/reservation_endpoints_tests.cpp 35c093567b07a11ca6eee85e2ff91894a460a7af
> src/tests/resources_tests.cpp 4b25e82c13e4f46c73803f773db90f269c09c48a
> src/v1/mesos.cpp 21b14cf7050270facab1ba2b844c3fde664665a4
> src/v1/resources.cpp be4a5d153e9313cb71a6e85d1ed25a358537f2b7
>
> Diff: https://reviews.apache.org/r/42754/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 42754: Added support for labels to resource
reservations.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42754/
-----------------------------------------------------------
(Updated Feb. 5, 2016, 2:19 a.m.)
Review request for mesos and Michael Park.
Bugs: MESOS-4479
https://issues.apache.org/jira/browse/MESOS-4479
Repository: mesos
Description
-------
Labels are free-form key-value pairs that can be used to associate
metadata with reserved resources.
Diffs (updated)
-----
include/mesos/mesos.proto 5d1a20b4ff647f43701a353472b56a4b74b6bbc3
include/mesos/type_utils.hpp 16f9cda83edd5064527b89e94392a7a111154411
include/mesos/v1/mesos.hpp b294f022345dc892c0581622c5b389efbde911a9
include/mesos/v1/mesos.proto 415a20d9f3385e9627635dd89d78c58d03e3c116
src/common/resources.cpp 588a279c3cdebbeb58047bfbff5e78a42f53fc13
src/common/type_utils.cpp 42e3061818c49c6b62d4fee2479db078dfe0fc89
src/tests/mesos.hpp e07d8aa6f1e507a91ce70763aafa134bdd9a7ec8
src/tests/reservation_endpoints_tests.cpp 35c093567b07a11ca6eee85e2ff91894a460a7af
src/tests/resources_tests.cpp 4b25e82c13e4f46c73803f773db90f269c09c48a
src/v1/mesos.cpp 21b14cf7050270facab1ba2b844c3fde664665a4
src/v1/resources.cpp be4a5d153e9313cb71a6e85d1ed25a358537f2b7
Diff: https://reviews.apache.org/r/42754/diff/
Testing
-------
make check
Thanks,
Neil Conway
Re: Review Request 42754: Added support for labels to resource
reservations.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42754/
-----------------------------------------------------------
(Updated Feb. 3, 2016, 11:32 p.m.)
Review request for mesos and Michael Park.
Changes
-------
Address code review comments from Joerg.
Bugs: MESOS-4479
https://issues.apache.org/jira/browse/MESOS-4479
Repository: mesos
Description
-------
Labels are free-form key-value pairs that can be used to associate
metadata with reserved resources.
Diffs (updated)
-----
include/mesos/mesos.proto 194750e92020753e60154083a47bdc3398d31466
include/mesos/type_utils.hpp 16f9cda83edd5064527b89e94392a7a111154411
include/mesos/v1/mesos.hpp b294f022345dc892c0581622c5b389efbde911a9
include/mesos/v1/mesos.proto 1102bbc92f46f97c1915c03a71c7cf829003e0ed
src/common/resources.cpp 588a279c3cdebbeb58047bfbff5e78a42f53fc13
src/common/type_utils.cpp 42e3061818c49c6b62d4fee2479db078dfe0fc89
src/tests/mesos.hpp c2bae4767ee7372c796bfad44ed1e86db7dd3488
src/tests/reservation_endpoints_tests.cpp 35c093567b07a11ca6eee85e2ff91894a460a7af
src/tests/resources_tests.cpp 4b25e82c13e4f46c73803f773db90f269c09c48a
src/v1/mesos.cpp 21b14cf7050270facab1ba2b844c3fde664665a4
src/v1/resources.cpp be4a5d153e9313cb71a6e85d1ed25a358537f2b7
Diff: https://reviews.apache.org/r/42754/diff/
Testing
-------
make check
Thanks,
Neil Conway
Re: Review Request 42754: Added support for labels to resource
reservations.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42754/
-----------------------------------------------------------
(Updated Feb. 3, 2016, 11:05 p.m.)
Review request for mesos and Michael Park.
Changes
-------
Rebase.
Bugs: MESOS-4479
https://issues.apache.org/jira/browse/MESOS-4479
Repository: mesos
Description
-------
Labels are free-form key-value pairs that can be used to associate
metadata with reserved resources.
Diffs (updated)
-----
include/mesos/mesos.proto 194750e92020753e60154083a47bdc3398d31466
include/mesos/type_utils.hpp 16f9cda83edd5064527b89e94392a7a111154411
include/mesos/v1/mesos.hpp b294f022345dc892c0581622c5b389efbde911a9
include/mesos/v1/mesos.proto 1102bbc92f46f97c1915c03a71c7cf829003e0ed
src/common/resources.cpp 588a279c3cdebbeb58047bfbff5e78a42f53fc13
src/common/type_utils.cpp 42e3061818c49c6b62d4fee2479db078dfe0fc89
src/tests/mesos.hpp c2bae4767ee7372c796bfad44ed1e86db7dd3488
src/tests/reservation_endpoints_tests.cpp 35c093567b07a11ca6eee85e2ff91894a460a7af
src/tests/resources_tests.cpp 4b25e82c13e4f46c73803f773db90f269c09c48a
src/v1/mesos.cpp 21b14cf7050270facab1ba2b844c3fde664665a4
src/v1/resources.cpp be4a5d153e9313cb71a6e85d1ed25a358537f2b7
Diff: https://reviews.apache.org/r/42754/diff/
Testing
-------
make check
Thanks,
Neil Conway
Re: Review Request 42754: Added support for labels to resource
reservations.
Posted by Joerg Schad <jo...@mesosphere.io>.
> On Feb. 3, 2016, 8:38 p.m., Joerg Schad wrote:
> > src/tests/resources_tests.cpp, line 1756
> > <https://reviews.apache.org/r/42754/diff/2/?file=1221650#file1221650line1756>
> >
> > Isn't this already checked above?
>
> Neil Conway wrote:
> Hmm -- where?
In the Substraction test case above.
- Joerg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42754/#review117667
-----------------------------------------------------------
On Feb. 3, 2016, 11:32 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42754/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2016, 11:32 p.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Bugs: MESOS-4479
> https://issues.apache.org/jira/browse/MESOS-4479
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Labels are free-form key-value pairs that can be used to associate
> metadata with reserved resources.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 194750e92020753e60154083a47bdc3398d31466
> include/mesos/type_utils.hpp 16f9cda83edd5064527b89e94392a7a111154411
> include/mesos/v1/mesos.hpp b294f022345dc892c0581622c5b389efbde911a9
> include/mesos/v1/mesos.proto 1102bbc92f46f97c1915c03a71c7cf829003e0ed
> src/common/resources.cpp 588a279c3cdebbeb58047bfbff5e78a42f53fc13
> src/common/type_utils.cpp 42e3061818c49c6b62d4fee2479db078dfe0fc89
> src/tests/mesos.hpp c2bae4767ee7372c796bfad44ed1e86db7dd3488
> src/tests/reservation_endpoints_tests.cpp 35c093567b07a11ca6eee85e2ff91894a460a7af
> src/tests/resources_tests.cpp 4b25e82c13e4f46c73803f773db90f269c09c48a
> src/v1/mesos.cpp 21b14cf7050270facab1ba2b844c3fde664665a4
> src/v1/resources.cpp be4a5d153e9313cb71a6e85d1ed25a358537f2b7
>
> Diff: https://reviews.apache.org/r/42754/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 42754: Added support for labels to resource
reservations.
Posted by Neil Conway <ne...@gmail.com>.
> On Feb. 3, 2016, 8:38 p.m., Joerg Schad wrote:
> > src/tests/resources_tests.cpp, line 1756
> > <https://reviews.apache.org/r/42754/diff/2/?file=1221650#file1221650line1756>
> >
> > Isn't this already checked above?
Hmm -- where?
> On Feb. 3, 2016, 8:38 p.m., Joerg Schad wrote:
> > src/tests/resources_tests.cpp, line 1666
> > <https://reviews.apache.org/r/42754/diff/2/?file=1221650#file1221650line1666>
> >
> > Even though this is not done in the rest of the file, a short comment explaining the tests intention is helpful and present in other test files. Feel free to drop if you disagree.
FWIW, I'm inclined to omit comments when the code / test case is self-explanatory, which I think it is for most of these test cases. Happy to add them if people disagree though.
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42754/#review117667
-----------------------------------------------------------
On Feb. 3, 2016, 11:32 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42754/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2016, 11:32 p.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Bugs: MESOS-4479
> https://issues.apache.org/jira/browse/MESOS-4479
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Labels are free-form key-value pairs that can be used to associate
> metadata with reserved resources.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 194750e92020753e60154083a47bdc3398d31466
> include/mesos/type_utils.hpp 16f9cda83edd5064527b89e94392a7a111154411
> include/mesos/v1/mesos.hpp b294f022345dc892c0581622c5b389efbde911a9
> include/mesos/v1/mesos.proto 1102bbc92f46f97c1915c03a71c7cf829003e0ed
> src/common/resources.cpp 588a279c3cdebbeb58047bfbff5e78a42f53fc13
> src/common/type_utils.cpp 42e3061818c49c6b62d4fee2479db078dfe0fc89
> src/tests/mesos.hpp c2bae4767ee7372c796bfad44ed1e86db7dd3488
> src/tests/reservation_endpoints_tests.cpp 35c093567b07a11ca6eee85e2ff91894a460a7af
> src/tests/resources_tests.cpp 4b25e82c13e4f46c73803f773db90f269c09c48a
> src/v1/mesos.cpp 21b14cf7050270facab1ba2b844c3fde664665a4
> src/v1/resources.cpp be4a5d153e9313cb71a6e85d1ed25a358537f2b7
>
> Diff: https://reviews.apache.org/r/42754/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 42754: Added support for labels to resource
reservations.
Posted by Joerg Schad <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42754/#review117667
-----------------------------------------------------------
src/tests/reservation_endpoints_tests.cpp (line 754)
<https://reviews.apache.org/r/42754/#comment178948>
just for my understanding: the filter does not effect reserved resources (as we still expect labeledResource2 to be offered below), correct?
src/tests/resources_tests.cpp (line 1623)
<https://reviews.apache.org/r/42754/#comment178935>
Shouldn't we also test that different labels are not considered equal?
src/tests/resources_tests.cpp (line 1666)
<https://reviews.apache.org/r/42754/#comment178936>
Even though this is not done in the rest of the file, a short comment explaining the tests intention is helpful and present in other test files. Feel free to drop if you disagree.
src/tests/resources_tests.cpp (line 1685)
<https://reviews.apache.org/r/42754/#comment178937>
see above, short comment of test intention.
src/tests/resources_tests.cpp (line 1740)
<https://reviews.apache.org/r/42754/#comment178938>
see above. Short comment describing tests intention.
src/tests/resources_tests.cpp (line 1756)
<https://reviews.apache.org/r/42754/#comment178943>
Isn't this already checked above?
src/tests/resources_tests.cpp (line 1757)
<https://reviews.apache.org/r/42754/#comment178942>
Checking each time the inverse case seems a little superfluous as both Resources only differ in the label. Still not a real issue for me
src/v1/resources.cpp (line 1494)
<https://reviews.apache.org/r/42754/#comment178939>
Fine with me, but in the case of volume we have one blank line between the last `stream <<` and return statement.
- Joerg Schad
On Feb. 3, 2016, 5:51 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42754/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2016, 5:51 p.m.)
>
>
> Review request for mesos and Michael Park.
>
>
> Bugs: MESOS-4479
> https://issues.apache.org/jira/browse/MESOS-4479
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Labels are free-form key-value pairs that can be used to associate
> metadata with reserved resources.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 194750e92020753e60154083a47bdc3398d31466
> include/mesos/type_utils.hpp 16f9cda83edd5064527b89e94392a7a111154411
> include/mesos/v1/mesos.hpp b294f022345dc892c0581622c5b389efbde911a9
> include/mesos/v1/mesos.proto 1102bbc92f46f97c1915c03a71c7cf829003e0ed
> src/common/resources.cpp 588a279c3cdebbeb58047bfbff5e78a42f53fc13
> src/common/type_utils.cpp 42e3061818c49c6b62d4fee2479db078dfe0fc89
> src/tests/mesos.hpp c2bae4767ee7372c796bfad44ed1e86db7dd3488
> src/tests/reservation_endpoints_tests.cpp 35c093567b07a11ca6eee85e2ff91894a460a7af
> src/tests/resources_tests.cpp 4b25e82c13e4f46c73803f773db90f269c09c48a
> src/v1/mesos.cpp 21b14cf7050270facab1ba2b844c3fde664665a4
> src/v1/resources.cpp be4a5d153e9313cb71a6e85d1ed25a358537f2b7
>
> Diff: https://reviews.apache.org/r/42754/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Neil Conway
>
>