You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Kapil Arya <ka...@mesosphere.io> on 2015/09/14 22:55:34 UTC
Review Request 38366: Added helper to model Labels message for
state.json.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38366/
-----------------------------------------------------------
Review request for mesos, Connor Doyle, Jie Yu, and Niklas Nielsen.
Repository: mesos
Description
-------
Also updated Task modelling to show labels only if Task.has_labels() is true. This way, the "labels" field won't shown if there are no labels. This makes it consistent with the modelling of rest of the "optional" fields.
Diffs
-----
src/common/http.hpp 61ad5314fb14cab0d1cec4fb855fb89b7ac8cf60
src/common/http.cpp 9c0d31e261788c492224345b9e0c32643ddb4156
src/common/protobuf_utils.cpp 08612700c456017638a9978e5fe9cfa466294c46
src/master/master.cpp 5589eca4317b597de509f3387cfc349083b361ac
src/tests/common/http_tests.cpp bf8712b11339b409514ab86c1f32eaf7e9c9a2f1
Diff: https://reviews.apache.org/r/38366/diff/
Testing
-------
make check
Thanks,
Kapil Arya
Re: Review Request 38366: Added helper to model Labels message for
state.json.
Posted by Kapil Arya <ka...@mesosphere.io>.
> On Sept. 15, 2015, 1:56 p.m., Niklas Nielsen wrote:
> > src/common/protobuf_utils.cpp, line 140
> > <https://reviews.apache.org/r/38366/diff/1/?file=1072947#file1072947line140>
> >
> > This could break existing 3rd party parsing; why not leave it set?
I am not so sure. This is fairly inconsistent with the rest of the format. Is there is a guideline that we can follow for such changes?
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38366/#review99051
-----------------------------------------------------------
On Sept. 14, 2015, 4:55 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38366/
> -----------------------------------------------------------
>
> (Updated Sept. 14, 2015, 4:55 p.m.)
>
>
> Review request for mesos, Connor Doyle, Jie Yu, and Niklas Nielsen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Also updated Task modelling to show labels only if Task.has_labels() is true. This way, the "labels" field won't shown if there are no labels. This makes it consistent with the modelling of rest of the "optional" fields.
>
>
> Diffs
> -----
>
> src/common/http.hpp 61ad5314fb14cab0d1cec4fb855fb89b7ac8cf60
> src/common/http.cpp 9c0d31e261788c492224345b9e0c32643ddb4156
> src/common/protobuf_utils.cpp 08612700c456017638a9978e5fe9cfa466294c46
> src/master/master.cpp 5589eca4317b597de509f3387cfc349083b361ac
> src/tests/common/http_tests.cpp bf8712b11339b409514ab86c1f32eaf7e9c9a2f1
>
> Diff: https://reviews.apache.org/r/38366/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 38366: Added helper to model Labels message for
state.json.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38366/#review99051
-----------------------------------------------------------
Alrighty; mind adding a comment above the new model() describing that we do this to avoid the nested 'labels: labels: [ ... ]' by just using JSON::protobuf()?
src/common/protobuf_utils.cpp (line 140)
<https://reviews.apache.org/r/38366/#comment155883>
This could break existing 3rd party parsing; why not leave it set?
- Niklas Nielsen
On Sept. 14, 2015, 1:55 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38366/
> -----------------------------------------------------------
>
> (Updated Sept. 14, 2015, 1:55 p.m.)
>
>
> Review request for mesos, Connor Doyle, Jie Yu, and Niklas Nielsen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Also updated Task modelling to show labels only if Task.has_labels() is true. This way, the "labels" field won't shown if there are no labels. This makes it consistent with the modelling of rest of the "optional" fields.
>
>
> Diffs
> -----
>
> src/common/http.hpp 61ad5314fb14cab0d1cec4fb855fb89b7ac8cf60
> src/common/http.cpp 9c0d31e261788c492224345b9e0c32643ddb4156
> src/common/protobuf_utils.cpp 08612700c456017638a9978e5fe9cfa466294c46
> src/master/master.cpp 5589eca4317b597de509f3387cfc349083b361ac
> src/tests/common/http_tests.cpp bf8712b11339b409514ab86c1f32eaf7e9c9a2f1
>
> Diff: https://reviews.apache.org/r/38366/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 38366: Added helper to model Labels message for
state.json.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38366/
-----------------------------------------------------------
(Updated Sept. 16, 2015, 6 p.m.)
Review request for mesos, Connor Doyle, Jie Yu, and Niklas Nielsen.
Changes
-------
rebased
Repository: mesos
Description
-------
Also updated Task modelling to show labels only if Task.has_labels() is true. This way, the "labels" field won't shown if there are no labels. This makes it consistent with the modelling of rest of the "optional" fields.
Diffs (updated)
-----
src/common/http.hpp 61ad5314fb14cab0d1cec4fb855fb89b7ac8cf60
src/common/http.cpp 9c0d31e261788c492224345b9e0c32643ddb4156
src/common/protobuf_utils.cpp 08612700c456017638a9978e5fe9cfa466294c46
src/master/master.cpp f26271c5b21685916c0654488ac1464f21d72e9a
src/tests/common/http_tests.cpp bf8712b11339b409514ab86c1f32eaf7e9c9a2f1
Diff: https://reviews.apache.org/r/38366/diff/
Testing
-------
make check
Thanks,
Kapil Arya
Re: Review Request 38366: Added helper to model Labels message for
state.json.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38366/#review99282
-----------------------------------------------------------
Ship it!
Ship It!
- Niklas Nielsen
On Sept. 14, 2015, 1:55 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38366/
> -----------------------------------------------------------
>
> (Updated Sept. 14, 2015, 1:55 p.m.)
>
>
> Review request for mesos, Connor Doyle, Jie Yu, and Niklas Nielsen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Also updated Task modelling to show labels only if Task.has_labels() is true. This way, the "labels" field won't shown if there are no labels. This makes it consistent with the modelling of rest of the "optional" fields.
>
>
> Diffs
> -----
>
> src/common/http.hpp 61ad5314fb14cab0d1cec4fb855fb89b7ac8cf60
> src/common/http.cpp 9c0d31e261788c492224345b9e0c32643ddb4156
> src/common/protobuf_utils.cpp 08612700c456017638a9978e5fe9cfa466294c46
> src/master/master.cpp 5589eca4317b597de509f3387cfc349083b361ac
> src/tests/common/http_tests.cpp bf8712b11339b409514ab86c1f32eaf7e9c9a2f1
>
> Diff: https://reviews.apache.org/r/38366/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 38366: Added helper to model Labels message for
state.json.
Posted by Kapil Arya <ka...@mesosphere.io>.
> On Sept. 14, 2015, 5:40 p.m., Niklas Nielsen wrote:
> > High level comment; would it be worth change JSON::Protobuf() to support Labels instead?
Probably not. We are trying a shortcut when modelling Labels for state.json. Thus, I wouldn't want to change JSON::Protobuf which is supposed to translate Protobufs to JSONs.
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38366/#review98922
-----------------------------------------------------------
On Sept. 14, 2015, 4:55 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38366/
> -----------------------------------------------------------
>
> (Updated Sept. 14, 2015, 4:55 p.m.)
>
>
> Review request for mesos, Connor Doyle, Jie Yu, and Niklas Nielsen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Also updated Task modelling to show labels only if Task.has_labels() is true. This way, the "labels" field won't shown if there are no labels. This makes it consistent with the modelling of rest of the "optional" fields.
>
>
> Diffs
> -----
>
> src/common/http.hpp 61ad5314fb14cab0d1cec4fb855fb89b7ac8cf60
> src/common/http.cpp 9c0d31e261788c492224345b9e0c32643ddb4156
> src/common/protobuf_utils.cpp 08612700c456017638a9978e5fe9cfa466294c46
> src/master/master.cpp 5589eca4317b597de509f3387cfc349083b361ac
> src/tests/common/http_tests.cpp bf8712b11339b409514ab86c1f32eaf7e9c9a2f1
>
> Diff: https://reviews.apache.org/r/38366/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 38366: Added helper to model Labels message for
state.json.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38366/#review98922
-----------------------------------------------------------
High level comment; would it be worth change JSON::Protobuf() to support Labels instead?
- Niklas Nielsen
On Sept. 14, 2015, 1:55 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38366/
> -----------------------------------------------------------
>
> (Updated Sept. 14, 2015, 1:55 p.m.)
>
>
> Review request for mesos, Connor Doyle, Jie Yu, and Niklas Nielsen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Also updated Task modelling to show labels only if Task.has_labels() is true. This way, the "labels" field won't shown if there are no labels. This makes it consistent with the modelling of rest of the "optional" fields.
>
>
> Diffs
> -----
>
> src/common/http.hpp 61ad5314fb14cab0d1cec4fb855fb89b7ac8cf60
> src/common/http.cpp 9c0d31e261788c492224345b9e0c32643ddb4156
> src/common/protobuf_utils.cpp 08612700c456017638a9978e5fe9cfa466294c46
> src/master/master.cpp 5589eca4317b597de509f3387cfc349083b361ac
> src/tests/common/http_tests.cpp bf8712b11339b409514ab86c1f32eaf7e9c9a2f1
>
> Diff: https://reviews.apache.org/r/38366/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Kapil Arya
>
>