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 Bannier <be...@mesosphere.io> on 2017/05/03 12:08:33 UTC
Review Request 58949: WIP: Introduced ResourceProviderInfo proto.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58949/
-----------------------------------------------------------
Review request for mesos, Jie Yu and Jan Schlicht.
Bugs: MESOS-7388
https://issues.apache.org/jira/browse/MESOS-7388
Repository: mesos
Description
-------
Introduced ResourceProviderInfo proto.
Diffs
-----
include/mesos/mesos.proto 1935f47a52840f6b395ecb2d28296666551fa691
include/mesos/type_utils.hpp 4f0a59cf7bc4172eb98db01cf73bf285c931b4d0
include/mesos/v1/mesos.hpp e665ce7046eb6e9c4f0e896e4f8cf9e976c40454
include/mesos/v1/mesos.proto c7f0bec5c96f2f41344d4261d0696f9fe0421db7
src/common/type_utils.cpp 6bfb558d9f7dcdf921acdf737515a25c3a0bdaa0
src/v1/mesos.cpp babe39e0e29ae7ff35360d9dfdabf771fdc940ea
Diff: https://reviews.apache.org/r/58949/diff/1/
Testing
-------
Thanks,
Benjamin Bannier
Re: Review Request 58949: Introduced ResourceProviderInfo proto.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On May 8, 2017, 4:59 p.m., Jie Yu wrote:
> > src/common/type_utils.cpp
> > Lines 372-377 (patched)
> > <https://reviews.apache.org/r/58949/diff/2/?file=1710389#file1710389line372>
> >
> > Let's don't introduce this yet until it's actually being used. For that reason, we don't have that for AgentInfo in v1.
I believe we should add this particular operator just for symmetry. `operator==` will immediately be useful to the first users.
> On May 8, 2017, 4:59 p.m., Jie Yu wrote:
> > src/v1/mesos.cpp
> > Lines 374-379 (patched)
> > <https://reviews.apache.org/r/58949/diff/2/?file=1710390#file1710390line374>
> >
> > Ditto. Remove
Ditto. See above.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58949/#review174168
-----------------------------------------------------------
On May 8, 2017, 4:49 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58949/
> -----------------------------------------------------------
>
> (Updated May 8, 2017, 4:49 p.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Bugs: MESOS-7388
> https://issues.apache.org/jira/browse/MESOS-7388
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduced ResourceProviderInfo proto.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 1935f47a52840f6b395ecb2d28296666551fa691
> include/mesos/type_utils.hpp 4f0a59cf7bc4172eb98db01cf73bf285c931b4d0
> include/mesos/v1/mesos.hpp e665ce7046eb6e9c4f0e896e4f8cf9e976c40454
> include/mesos/v1/mesos.proto c7f0bec5c96f2f41344d4261d0696f9fe0421db7
> src/common/type_utils.cpp 6bfb558d9f7dcdf921acdf737515a25c3a0bdaa0
> src/v1/mesos.cpp babe39e0e29ae7ff35360d9dfdabf771fdc940ea
>
>
> Diff: https://reviews.apache.org/r/58949/diff/2/
>
>
> Testing
> -------
>
> `make check` (Fedora25, clang-5)
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 58949: Introduced ResourceProviderInfo proto.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58949/#review174168
-----------------------------------------------------------
Fix it, then Ship it!
include/mesos/mesos.proto
Line 107 (original), 107-116 (patched)
<https://reviews.apache.org/r/58949/#comment247284>
I'll move this right below `SlaveInfo` for consistency. Looks like for the proto file, we use logical grouping. We put ID and basic type definitions first, and then the rest.
include/mesos/v1/mesos.proto
Lines 112 (patched)
<https://reviews.apache.org/r/58949/#comment247288>
Ditto on ordering.
src/common/type_utils.cpp
Lines 372-377 (patched)
<https://reviews.apache.org/r/58949/#comment247285>
Let's don't introduce this yet until it's actually being used. For that reason, we don't have that for AgentInfo in v1.
src/v1/mesos.cpp
Lines 346-353 (original), 346-354 (patched)
<https://reviews.apache.org/r/58949/#comment247286>
Can you swap the ordering between these two? Make sure the order in the cpp file is consistent with that in the header.
src/v1/mesos.cpp
Lines 374-379 (patched)
<https://reviews.apache.org/r/58949/#comment247287>
Ditto. Remove
- Jie Yu
On May 8, 2017, 2:49 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58949/
> -----------------------------------------------------------
>
> (Updated May 8, 2017, 2:49 p.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Bugs: MESOS-7388
> https://issues.apache.org/jira/browse/MESOS-7388
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduced ResourceProviderInfo proto.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 1935f47a52840f6b395ecb2d28296666551fa691
> include/mesos/type_utils.hpp 4f0a59cf7bc4172eb98db01cf73bf285c931b4d0
> include/mesos/v1/mesos.hpp e665ce7046eb6e9c4f0e896e4f8cf9e976c40454
> include/mesos/v1/mesos.proto c7f0bec5c96f2f41344d4261d0696f9fe0421db7
> src/common/type_utils.cpp 6bfb558d9f7dcdf921acdf737515a25c3a0bdaa0
> src/v1/mesos.cpp babe39e0e29ae7ff35360d9dfdabf771fdc940ea
>
>
> Diff: https://reviews.apache.org/r/58949/diff/2/
>
>
> Testing
> -------
>
> `make check` (Fedora25, clang-5)
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 58949: Introduced ResourceProviderInfo proto.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58949/#review174274
-----------------------------------------------------------
Ship it!
Ship It!
- Jan Schlicht
On May 9, 2017, 9:58 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58949/
> -----------------------------------------------------------
>
> (Updated May 9, 2017, 9:58 a.m.)
>
>
> Review request for mesos, Jie Yu and Jan Schlicht.
>
>
> Bugs: MESOS-7388
> https://issues.apache.org/jira/browse/MESOS-7388
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Introduced ResourceProviderInfo proto.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 1935f47a52840f6b395ecb2d28296666551fa691
> include/mesos/type_utils.hpp 4f0a59cf7bc4172eb98db01cf73bf285c931b4d0
> include/mesos/v1/mesos.hpp e665ce7046eb6e9c4f0e896e4f8cf9e976c40454
> include/mesos/v1/mesos.proto c7f0bec5c96f2f41344d4261d0696f9fe0421db7
> src/common/type_utils.cpp 6bfb558d9f7dcdf921acdf737515a25c3a0bdaa0
> src/v1/mesos.cpp babe39e0e29ae7ff35360d9dfdabf771fdc940ea
>
>
> Diff: https://reviews.apache.org/r/58949/diff/3/
>
>
> Testing
> -------
>
> `make check` (Fedora25, clang-5)
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 58949: Introduced ResourceProviderInfo proto.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58949/
-----------------------------------------------------------
(Updated May 9, 2017, 9:58 a.m.)
Review request for mesos, Jie Yu and Jan Schlicht.
Changes
-------
Addressed Jie's comments.
Bugs: MESOS-7388
https://issues.apache.org/jira/browse/MESOS-7388
Repository: mesos
Description
-------
Introduced ResourceProviderInfo proto.
Diffs (updated)
-----
include/mesos/mesos.proto 1935f47a52840f6b395ecb2d28296666551fa691
include/mesos/type_utils.hpp 4f0a59cf7bc4172eb98db01cf73bf285c931b4d0
include/mesos/v1/mesos.hpp e665ce7046eb6e9c4f0e896e4f8cf9e976c40454
include/mesos/v1/mesos.proto c7f0bec5c96f2f41344d4261d0696f9fe0421db7
src/common/type_utils.cpp 6bfb558d9f7dcdf921acdf737515a25c3a0bdaa0
src/v1/mesos.cpp babe39e0e29ae7ff35360d9dfdabf771fdc940ea
Diff: https://reviews.apache.org/r/58949/diff/3/
Changes: https://reviews.apache.org/r/58949/diff/2-3/
Testing
-------
`make check` (Fedora25, clang-5)
Thanks,
Benjamin Bannier
Re: Review Request 58949: Introduced ResourceProviderInfo proto.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58949/
-----------------------------------------------------------
(Updated May 8, 2017, 4:49 p.m.)
Review request for mesos, Jie Yu and Jan Schlicht.
Changes
-------
Track agent_id separate from ResourceProviderInfo.
Summary (updated)
-----------------
Introduced ResourceProviderInfo proto.
Bugs: MESOS-7388
https://issues.apache.org/jira/browse/MESOS-7388
Repository: mesos
Description
-------
Introduced ResourceProviderInfo proto.
Diffs (updated)
-----
include/mesos/mesos.proto 1935f47a52840f6b395ecb2d28296666551fa691
include/mesos/type_utils.hpp 4f0a59cf7bc4172eb98db01cf73bf285c931b4d0
include/mesos/v1/mesos.hpp e665ce7046eb6e9c4f0e896e4f8cf9e976c40454
include/mesos/v1/mesos.proto c7f0bec5c96f2f41344d4261d0696f9fe0421db7
src/common/type_utils.cpp 6bfb558d9f7dcdf921acdf737515a25c3a0bdaa0
src/v1/mesos.cpp babe39e0e29ae7ff35360d9dfdabf771fdc940ea
Diff: https://reviews.apache.org/r/58949/diff/2/
Changes: https://reviews.apache.org/r/58949/diff/1-2/
Testing (updated)
-------
`make check` (Fedora25, clang-5)
Thanks,
Benjamin Bannier