Re: Review Request 41188: Providing JSON bindings to so that mesos modules can expose DiscoveryInfo protobuf messages to HTTP endpoints

[Adam B]

> On Dec. 16, 2015, 4:12 p.m., Anand Mazumdar wrote:
> > src/tests/common/http_tests.cpp, lines 130-140
> > 
> >
> > Can you confirm that other objects that have labels that are exposed 
> > via `/state` endpoint also have a nested `labels` mapping ?
> > 
> > AFAICT, this is an artifact of us using the Protobuf to JSON converters 
> > when we should have just used the `model(...)` functions.
> 
> Avinash sridharan wrote:
> I am trying to understand this comment. Is it that applications don't 
> expect nested labels (for ports) when they read state.json? 
> 
> In terms of exposing labels in state.json I can see from 
> TEST_F(SlaveTest, TaskLabels) (slave_tests.cpp) the labels are nested within 
> tasks. Since, DiscoveryInfo also has labels and Port has labels unless we 
> nest them, the relationship would not be explicit if we do not replicate the 
> protobuf right?
> 
> Anand Mazumdar wrote:
> Let me give you an example:
> 
> Here is how the Task Labels JSON returned from this test looks like:
> 
> ```
> "tasks": [
> {
>   "executor_id": "default",
>   "framework_id": "5452180f-fa83-4d25-802f-8a0c974dfd21-",
>   "id": "1",
>   "labels": [
> {
>   "key": "foo",
>   "value": "bar"
> },
> {
>   "key": "bar",
>   "value": "baz"
> },
> {
>   "key": "bar",
>   "value": "qux"
> }
>   ],
>   "name": "",
>   "resources": {
> "cpus": 2,
> "disk": 1024,
> "mem": 1024,
> "ports": "[31000-32000]"
>   },
> ```
> 
> If you notice, it does not have a "labels" field nested inside a "labels" 
> field as we have when we do automated protobuf to JSON conversions. Do we 
> want to preserve this behavior ?
> 
> Avinash sridharan wrote:
> I get your point . You are right this is an artifact of using the 
> JSON::protobuf compred to explicit model functions . 
> 
> Is this going to be an issue? Since the dependency is reflected in the 
> protobuf definition.
> One reason I can of think of keeping it as is, is that the protobuf is a 
> contract between the framework and service discovery, so they might actually 
> expect this.

Note that your proposed changes also have "ports" nested under "ports".
Arguments for the compact (no nesting) format:
+ App: Shorter find/get query strings, more compact json output to read.
+ Perf: Less json to generate, fewer bytes over the wire.
+ Not matching exact protobuf format means we can break the format easier later.
Arguments for nested format:
+ Direct json translation to/from protobuf; otherwise, all clients/readers will 
have to manually update their parsing (in a potentially undocumented manner) 
anytime the data is modeled differently.

Also note that this nested protobuf pattern is one of the less fortunate side 
effects of having an `optional Labels labels` field instead of directly using a 
`repeated Label labels` field. Maybe we should switch to the `repeated` pattern 
for all labels/ports so we can use the direct json/protobuf translation 
functions for everything?


- Adam


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41188/#review110804
---


On Dec. 16, 2015, 7:10 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41188/
> ---
> 
> (Updated Dec. 16, 2015, 7:10 p.m.)
> 
> 
> Review request for mesos, Adam B and Anand Mazumdar.
> 
> 
> Bugs: MESOS-3962
> https://issues.apache.org/jira/browse/MESOS-3962
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Providing JSON bindings to so that mesos modules can expose DiscoveryInfo 
> protobuf messages to HTTP endpoints
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp d1b1158c0a80d72c32c9b28977043b6be2295239 
>   src/tests/common/http_tests.cpp 3aca5b0437a012664f58ff331cc7cf682d442699 
>   src/tests/slave_tests.cpp 4975bea8a7a701e0414426760692720f73dea7f5 
> 
> Diff: https://reviews.apache.org/r/41188/diff/
> 
> 
> Testing
> ---
> 
> make check.
> Added Unit tests to verify setting of DiscoveryInfo in state.json in slave. 
> Also added Unit test to test that DiscoveryInfo gets exposed in master when 
> TaskInfo protobuf is converted to JSON objects.
> 
> 
> Thanks,

Re: Review Request 41188: Providing JSON bindings to so that mesos modules can expose DiscoveryInfo protobuf messages to HTTP endpoints

[Adam B]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41188/#review110927
---


Looks great! Barring the nested labels discussion, I just had a couple of nits 
and it's otherwise shippable.


src/tests/common/http_tests.cpp (lines 38 - 39)


Alphabetize, please



src/tests/common/http_tests.cpp (lines 81 - 82)


Need to set DiscoveryInfo.visibility, since it's `required`



src/tests/slave_tests.cpp (line 2169)


`Times(1)` is the default, unnecessary. Please delete.



src/tests/slave_tests.cpp (line 2184)


May want to use a different example than "vip", if we're about to add 
`vips` as a first-class field.



src/tests/slave_tests.cpp (lines 2190 - 2194)


s/`Port *port`/`Port* port`/



src/tests/slave_tests.cpp (line 2219)


Don't think you have to AWAIT on `response` here, since you AWAIT_EXPECT on 
it just below.



src/tests/slave_tests.cpp (lines  - 2224)


Doesn't this all fit on one 80-char line?

Also, it looks like we have a AWAIT_EXPECT_RESPONSE_HEADER_EQ, but it 
doesn't really save any typing (1char?), and you don't need to AWAIT again.


- Adam B


On Dec. 16, 2015, 7:10 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41188/
> ---
> 
> (Updated Dec. 16, 2015, 7:10 p.m.)
> 
> 
> Review request for mesos, Adam B and Anand Mazumdar.
> 
> 
> Bugs: MESOS-3962
> https://issues.apache.org/jira/browse/MESOS-3962
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Providing JSON bindings to so that mesos modules can expose DiscoveryInfo 
> protobuf messages to HTTP endpoints
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp d1b1158c0a80d72c32c9b28977043b6be2295239 
>   src/tests/common/http_tests.cpp 3aca5b0437a012664f58ff331cc7cf682d442699 
>   src/tests/slave_tests.cpp 4975bea8a7a701e0414426760692720f73dea7f5 
> 
> Diff: https://reviews.apache.org/r/41188/diff/
> 
> 
> Testing
> ---
> 
> make check.
> Added Unit tests to verify setting of DiscoveryInfo in state.json in slave. 
> Also added Unit test to test that DiscoveryInfo gets exposed in master when 
> TaskInfo protobuf is converted to JSON objects.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 41188: Providing JSON bindings to so that mesos modules can expose DiscoveryInfo protobuf messages to HTTP endpoints

[Avinash sridharan]

> On Dec. 17, 2015, 12:12 a.m., Anand Mazumdar wrote:
> > src/tests/common/http_tests.cpp, lines 130-140
> > 
> >
> > Can you confirm that other objects that have labels that are exposed 
> > via `/state` endpoint also have a nested `labels` mapping ?
> > 
> > AFAICT, this is an artifact of us using the Protobuf to JSON converters 
> > when we should have just used the `model(...)` functions.
> 
> Avinash sridharan wrote:
> I am trying to understand this comment. Is it that applications don't 
> expect nested labels (for ports) when they read state.json? 
> 
> In terms of exposing labels in state.json I can see from 
> TEST_F(SlaveTest, TaskLabels) (slave_tests.cpp) the labels are nested within 
> tasks. Since, DiscoveryInfo also has labels and Port has labels unless we 
> nest them, the relationship would not be explicit if we do not replicate the 
> protobuf right?
> 
> Anand Mazumdar wrote:
> Let me give you an example:
> 
> Here is how the Task Labels JSON returned from this test looks like:
> 
> ```
> "tasks": [
> {
>   "executor_id": "default",
>   "framework_id": "5452180f-fa83-4d25-802f-8a0c974dfd21-",
>   "id": "1",
>   "labels": [
> {
>   "key": "foo",
>   "value": "bar"
> },
> {
>   "key": "bar",
>   "value": "baz"
> },
> {
>   "key": "bar",
>   "value": "qux"
> }
>   ],
>   "name": "",
>   "resources": {
> "cpus": 2,
> "disk": 1024,
> "mem": 1024,
> "ports": "[31000-32000]"
>   },
> ```
> 
> If you notice, it does not have a "labels" field nested inside a "labels" 
> field as we have when we do automated protobuf to JSON conversions. Do we 
> want to preserve this behavior ?

I get your point . You are right this is an artifact of using the 
JSON::protobuf compred to explicit model functions . 

Is this going to be an issue? Since the dependency is reflected in the protobuf 
definition.
One reason I can of think of keeping it as is, is that the protobuf is a 
contract between the framework and service discovery, so they might actually 
expect this.


- Avinash


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41188/#review110804
---


On Dec. 17, 2015, 3:10 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41188/
> ---
> 
> (Updated Dec. 17, 2015, 3:10 a.m.)
> 
> 
> Review request for mesos, Adam B and Anand Mazumdar.
> 
> 
> Bugs: MESOS-3962
> https://issues.apache.org/jira/browse/MESOS-3962
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Providing JSON bindings to so that mesos modules can expose DiscoveryInfo 
> protobuf messages to HTTP endpoints
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp d1b1158c0a80d72c32c9b28977043b6be2295239 
>   src/tests/common/http_tests.cpp 3aca5b0437a012664f58ff331cc7cf682d442699 
>   src/tests/slave_tests.cpp 4975bea8a7a701e0414426760692720f73dea7f5 
> 
> Diff: https://reviews.apache.org/r/41188/diff/
> 
> 
> Testing
> ---
> 
> make check.
> Added Unit tests to verify setting of DiscoveryInfo in state.json in slave. 
> Also added Unit test to test that DiscoveryInfo gets exposed in master when 
> TaskInfo protobuf is converted to JSON objects.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 41188: Providing JSON bindings to so that mesos modules can expose DiscoveryInfo protobuf messages to HTTP endpoints

[Avinash sridharan]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41188/
---

(Updated Dec. 17, 2015, 3:10 a.m.)


Review request for mesos, Adam B and Anand Mazumdar.


Bugs: MESOS-3962
https://issues.apache.org/jira/browse/MESOS-3962


Repository: mesos


Description
---

Providing JSON bindings to so that mesos modules can expose DiscoveryInfo 
protobuf messages to HTTP endpoints


Diffs (updated)
-

  src/slave/http.cpp d1b1158c0a80d72c32c9b28977043b6be2295239 
  src/tests/common/http_tests.cpp 3aca5b0437a012664f58ff331cc7cf682d442699 
  src/tests/slave_tests.cpp 4975bea8a7a701e0414426760692720f73dea7f5 

Diff: https://reviews.apache.org/r/41188/diff/


Testing
---

make check.
Added Unit tests to verify setting of DiscoveryInfo in state.json in slave. 
Also added Unit test to test that DiscoveryInfo gets exposed in master when 
TaskInfo protobuf is converted to JSON objects.


Thanks,

Avinash sridharan

Re: Review Request 41188: Providing JSON bindings to so that mesos modules can expose DiscoveryInfo protobuf messages to HTTP endpoints

[Anand Mazumdar]

> On Dec. 17, 2015, 12:12 a.m., Anand Mazumdar wrote:
> > src/tests/common/http_tests.cpp, lines 130-140
> > 
> >
> > Can you confirm that other objects that have labels that are exposed 
> > via `/state` endpoint also have a nested `labels` mapping ?
> > 
> > AFAICT, this is an artifact of us using the Protobuf to JSON converters 
> > when we should have just used the `model(...)` functions.
> 
> Avinash sridharan wrote:
> I am trying to understand this comment. Is it that applications don't 
> expect nested labels (for ports) when they read state.json? 
> 
> In terms of exposing labels in state.json I can see from 
> TEST_F(SlaveTest, TaskLabels) (slave_tests.cpp) the labels are nested within 
> tasks. Since, DiscoveryInfo also has labels and Port has labels unless we 
> nest them, the relationship would not be explicit if we do not replicate the 
> protobuf right?

Let me give you an example:

Here is how the Task Labels JSON returned from this test looks like:

```
"tasks": [
{
  "executor_id": "default",
  "framework_id": "5452180f-fa83-4d25-802f-8a0c974dfd21-",
  "id": "1",
  "labels": [
{
  "key": "foo",
  "value": "bar"
},
{
  "key": "bar",
  "value": "baz"
},
{
  "key": "bar",
  "value": "qux"
}
  ],
  "name": "",
  "resources": {
"cpus": 2,
"disk": 1024,
"mem": 1024,
"ports": "[31000-32000]"
  },
```

If you notice, it does not have a "labels" field nested inside a "labels" field 
as we have when we do automated protobuf to JSON conversions. Do we want to 
preserve this behavior ?


- Anand


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41188/#review110804
---


On Dec. 15, 2015, 9:13 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41188/
> ---
> 
> (Updated Dec. 15, 2015, 9:13 p.m.)
> 
> 
> Review request for mesos, Adam B and Anand Mazumdar.
> 
> 
> Bugs: MESOS-3962
> https://issues.apache.org/jira/browse/MESOS-3962
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Providing JSON bindings to so that mesos modules can expose DiscoveryInfo 
> protobuf messages to HTTP endpoints
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp d1b1158c0a80d72c32c9b28977043b6be2295239 
>   src/tests/common/http_tests.cpp 3aca5b0437a012664f58ff331cc7cf682d442699 
>   src/tests/slave_tests.cpp 4975bea8a7a701e0414426760692720f73dea7f5 
> 
> Diff: https://reviews.apache.org/r/41188/diff/
> 
> 
> Testing
> ---
> 
> make check.
> Added Unit tests to verify setting of DiscoveryInfo in state.json in slave. 
> Also added Unit test to test that DiscoveryInfo gets exposed in master when 
> TaskInfo protobuf is converted to JSON objects.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 41188: Providing JSON bindings to so that mesos modules can expose DiscoveryInfo protobuf messages to HTTP endpoints

[Avinash sridharan]

> On Dec. 17, 2015, 12:12 a.m., Anand Mazumdar wrote:
> > src/tests/slave_tests.cpp, line 2223
> > 
> >
> > you should be able to use `APPLICATION_JSON` defined in 
> > `include/mesos/http.hpp`

will do .


> On Dec. 17, 2015, 12:12 a.m., Anand Mazumdar wrote:
> > src/tests/common/http_tests.cpp, lines 72-76
> > 
> >
> > Can you modify this based on my earlier comment around `Label` in the 
> > earlier test.

will do. Sorry should have modified this with previous changes.


> On Dec. 17, 2015, 12:12 a.m., Anand Mazumdar wrote:
> > src/tests/common/http_tests.cpp, lines 130-140
> > 
> >
> > Can you confirm that other objects that have labels that are exposed 
> > via `/state` endpoint also have a nested `labels` mapping ?
> > 
> > AFAICT, this is an artifact of us using the Protobuf to JSON converters 
> > when we should have just used the `model(...)` functions.

I am trying to understand this comment. Is it that applications don't expect 
nested labels (for ports) when they read state.json? 

In terms of exposing labels in state.json I can see from TEST_F(SlaveTest, 
TaskLabels) (slave_tests.cpp) the labels are nested within tasks. Since, 
DiscoveryInfo also has labels and Port has labels unless we nest them, the 
relationship would not be explicit if we do not replicate the protobuf right?


- Avinash


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41188/#review110804
---


On Dec. 15, 2015, 9:13 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41188/
> ---
> 
> (Updated Dec. 15, 2015, 9:13 p.m.)
> 
> 
> Review request for mesos, Adam B and Anand Mazumdar.
> 
> 
> Bugs: MESOS-3962
> https://issues.apache.org/jira/browse/MESOS-3962
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Providing JSON bindings to so that mesos modules can expose DiscoveryInfo 
> protobuf messages to HTTP endpoints
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp d1b1158c0a80d72c32c9b28977043b6be2295239 
>   src/tests/common/http_tests.cpp 3aca5b0437a012664f58ff331cc7cf682d442699 
>   src/tests/slave_tests.cpp 4975bea8a7a701e0414426760692720f73dea7f5 
> 
> Diff: https://reviews.apache.org/r/41188/diff/
> 
> 
> Testing
> ---
> 
> make check.
> Added Unit tests to verify setting of DiscoveryInfo in state.json in slave. 
> Also added Unit test to test that DiscoveryInfo gets exposed in master when 
> TaskInfo protobuf is converted to JSON objects.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 41188: Providing JSON bindings to so that mesos modules can expose DiscoveryInfo protobuf messages to HTTP endpoints

[Anand Mazumdar]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41188/#review110804
---


LGTM, Just one thing to confirm around nested `labels` field in expected JSON.


src/tests/common/http_tests.cpp (lines 72 - 76)


Can you modify this based on my earlier comment around `Label` in the 
earlier test.



src/tests/common/http_tests.cpp (lines 130 - 140)


Can you confirm that other objects that have labels that are exposed via 
`/state` endpoint also have a nested `labels` mapping ?

AFAICT, this is an artifact of us using the Protobuf to JSON converters 
when we should have just used the `model(...)` functions.



src/tests/slave_tests.cpp (line 2223)


you should be able to use `APPLICATION_JSON` defined in 
`include/mesos/http.hpp`


- Anand Mazumdar


On Dec. 15, 2015, 9:13 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41188/
> ---
> 
> (Updated Dec. 15, 2015, 9:13 p.m.)
> 
> 
> Review request for mesos, Adam B and Anand Mazumdar.
> 
> 
> Bugs: MESOS-3962
> https://issues.apache.org/jira/browse/MESOS-3962
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Providing JSON bindings to so that mesos modules can expose DiscoveryInfo 
> protobuf messages to HTTP endpoints
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp d1b1158c0a80d72c32c9b28977043b6be2295239 
>   src/tests/common/http_tests.cpp 3aca5b0437a012664f58ff331cc7cf682d442699 
>   src/tests/slave_tests.cpp 4975bea8a7a701e0414426760692720f73dea7f5 
> 
> Diff: https://reviews.apache.org/r/41188/diff/
> 
> 
> Testing
> ---
> 
> make check.
> Added Unit tests to verify setting of DiscoveryInfo in state.json in slave. 
> Also added Unit test to test that DiscoveryInfo gets exposed in master when 
> TaskInfo protobuf is converted to JSON objects.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 41188: Providing JSON bindings to so that mesos modules can expose DiscoveryInfo protobuf messages to HTTP endpoints

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41188/#review110112
---


Patch looks great!

Reviews applied: [41187, 41188]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 15, 2015, 9:13 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41188/
> ---
> 
> (Updated Dec. 15, 2015, 9:13 p.m.)
> 
> 
> Review request for mesos, Adam B and Anand Mazumdar.
> 
> 
> Bugs: MESOS-3962
> https://issues.apache.org/jira/browse/MESOS-3962
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Providing JSON bindings to so that mesos modules can expose DiscoveryInfo 
> protobuf messages to HTTP endpoints
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp d1b1158c0a80d72c32c9b28977043b6be2295239 
>   src/tests/common/http_tests.cpp 3aca5b0437a012664f58ff331cc7cf682d442699 
>   src/tests/slave_tests.cpp 4975bea8a7a701e0414426760692720f73dea7f5 
> 
> Diff: https://reviews.apache.org/r/41188/diff/
> 
> 
> Testing
> ---
> 
> make check.
> Added Unit tests to verify setting of DiscoveryInfo in state.json in slave. 
> Also added Unit test to test that DiscoveryInfo gets exposed in master when 
> TaskInfo protobuf is converted to JSON objects.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 41188: Providing JSON bindings to so that mesos modules can expose DiscoveryInfo protobuf messages to HTTP endpoints

[Avinash sridharan]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41188/
---

(Updated Dec. 15, 2015, 9:13 p.m.)


Review request for mesos, Adam B and Anand Mazumdar.


Bugs: MESOS-3962
https://issues.apache.org/jira/browse/MESOS-3962


Repository: mesos


Description
---

Providing JSON bindings to so that mesos modules can expose DiscoveryInfo 
protobuf messages to HTTP endpoints


Diffs (updated)
-

  src/slave/http.cpp d1b1158c0a80d72c32c9b28977043b6be2295239 
  src/tests/common/http_tests.cpp 3aca5b0437a012664f58ff331cc7cf682d442699 
  src/tests/slave_tests.cpp 4975bea8a7a701e0414426760692720f73dea7f5 

Diff: https://reviews.apache.org/r/41188/diff/


Testing
---

make check.
Added Unit tests to verify setting of DiscoveryInfo in state.json in slave. 
Also added Unit test to test that DiscoveryInfo gets exposed in master when 
TaskInfo protobuf is converted to JSON objects.


Thanks,

Avinash sridharan

Re: Review Request 41188: Providing JSON bindings to so that mesos modules can expose DiscoveryInfo protobuf messages to HTTP endpoints

[Avinash sridharan]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41188/
---

(Updated Dec. 15, 2015, 4:28 p.m.)


Review request for mesos, Adam B and Anand Mazumdar.


Bugs: MESOS-3962
https://issues.apache.org/jira/browse/MESOS-3962


Repository: mesos


Description
---

Providing JSON bindings to so that mesos modules can expose DiscoveryInfo 
protobuf messages to HTTP endpoints


Diffs (updated)
-

  src/slave/http.cpp d1b1158c0a80d72c32c9b28977043b6be2295239 
  src/tests/common/http_tests.cpp 3aca5b0437a012664f58ff331cc7cf682d442699 
  src/tests/slave_tests.cpp 4975bea8a7a701e0414426760692720f73dea7f5 

Diff: https://reviews.apache.org/r/41188/diff/


Testing
---

make check.
Added Unit tests to verify setting of DiscoveryInfo in state.json in slave. 
Also added Unit test to test that DiscoveryInfo gets exposed in master when 
TaskInfo protobuf is converted to JSON objects.


Thanks,

Avinash sridharan

Re: Review Request 41188: Providing JSON bindings to so that mesos modules can expose DiscoveryInfo protobuf messages to HTTP endpoints

[Avinash sridharan]

> On Dec. 15, 2015, 4:35 a.m., Anand Mazumdar wrote:
> > src/tests/slave_tests.cpp, line 2147
> > 
> >
> > I am assuming that we are testing that port labels are populated 
> > correctly in the `DiscoveryInfo` object. 
> > 
> > How about:
> > ```
> > // This test verifies that port labels are exposed over the slave state 
> > endpoint.
> > ```

Well actually it tests both DiscoveryInfo and port labels. Let me be a bit more 
explicit about it and change it to:
// This test verifies that DiscoveryInfo and Port messages, set in TaskInfo,
// are exposed over the slave state endpoint. The test launches a task with
// the DiscoveryInfo and Port message fields populated. It then makes an HTTP
// request to the state endpoint of the slave and retrieves the JSON data from
// the endpoint. The test passes if the DiscoveryInfo and Port message data in
// JSON matches the corresponding data set in the TaskInfo used to launch the
// task.


> On Dec. 15, 2015, 4:35 a.m., Anand Mazumdar wrote:
> > src/tests/slave_tests.cpp, line 2149
> > 
> >
> > s/DiscoveryInfoAndPorts/PortLabels

This test the setting of both DiscoveryInfo and Ports hence the function name.


> On Dec. 15, 2015, 4:35 a.m., Anand Mazumdar wrote:
> > src/tests/slave_tests.cpp, line 2205
> > 
> >
> > Why not just do this:
> > 
> > ```
> > EXPECT_CALL(exec, registered(_, _, _, _, _));
> > ```
> > 
> > AFAICT, you don't seem to be using any of the arguments from the 
> > `registered` callback. All you are interested in is that the expectation is 
> > fullfilled exactly once.

Agrred, I thought we were using the execDriver further down to get the response 
but we are not.


> On Dec. 15, 2015, 4:35 a.m., Anand Mazumdar wrote:
> > src/tests/slave_tests.cpp, line 2209
> > 
> >
> > You don't seem to be using `execTask` i.e. `TaskInfo` attributed. Why 
> > not just do:
> > 
> > ```
> > Future launchTask;
> > EXPECT_CALL(exec, launchTask(_, _))
> >   .WillOnce(FutureSatisfy(&launchTask));
> > ```
> > 
> > and then later do
> > 
> > ```AWAIT_READY(launchTask);```

Cool !! Still getting comfortable with Google mock . This is nice :)


> On Dec. 15, 2015, 4:35 a.m., Anand Mazumdar wrote:
> > src/tests/slave_tests.cpp, line 
> > 
> >
> > We generally avoid magic string constants.
> > 
> > Can you just do
> > 
> > ```
> > ASSERT_SOME_EQ(
> > APPLICATION_JSON,
> > response.get().headers.get("Content-Type"));
> > ```
> > 
> > Can we also check if the response code is OK() ?

This is pre-existing code from TEST_F(SlaveTest, TaskLabels) ?? They are using 
"application/json. Is APPLICATION_JSON defined somewhere?


> On Dec. 15, 2015, 4:35 a.m., Anand Mazumdar wrote:
> > src/tests/slave_tests.cpp, line 2245
> > 
> >
> > New line before this line.
> > 
> > Also,
> > 
> > `// Verify that the ports match.`

These are comments. Lets me more explicit than implicit. Think about it from a 
newbie's perspective more info is better. Might not be perfect but better.


> On Dec. 15, 2015, 4:35 a.m., Anand Mazumdar wrote:
> > src/tests/slave_tests.cpp, line 2190
> > 
> >
> > How about:
> > 
> > ```
> > Port *port1 = ports.add_ports();
> > port1->set_number(80);
> > port1->mutable_labels()->CopyFrom(labels1);
> > 
> > Port *port2 = ports.add_ports();
> > port2->set_number(8081);
> > port2->mutable_labels()->CopyFrom(labels2);
> > ```
> > 
> > This makes it a bit cleaner.

Yup looks cleaner :).


- Avinash


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41188/#review110387
---


On Dec. 14, 2015, 11:34 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41188/
> ---
> 
> (Updated Dec. 14, 2015, 11:34 p.m.)
> 
> 
> Review request for mesos, Adam B and Anand Mazumdar.
> 
> 
> Bugs: MESOS-3962
> https://issues.apache.org/jira/browse/MESOS-3962
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Providing JSON bindings to so that mesos modules can expose DiscoveryInfo 
> protobuf messa

Re: Review Request 41188: Providing JSON bindings to so that mesos modules can expose DiscoveryInfo protobuf messages to HTTP endpoints

[Anand Mazumdar]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41188/#review110387
---


LGTM, Just some minor style nits and suggestions.


src/tests/slave_tests.cpp (line 2147)


I am assuming that we are testing that port labels are populated correctly 
in the `DiscoveryInfo` object. 

How about:
```
// This test verifies that port labels are exposed over the slave state 
endpoint.
```



src/tests/slave_tests.cpp (line 2149)


s/DiscoveryInfoAndPorts/PortLabels



src/tests/slave_tests.cpp (line 2178)


```
labels->add_labels()->CopyFrom(createLabel("vip1", "192.168.1.1:80"));
```

Ditto for the other occurence.



src/tests/slave_tests.cpp (line 2188)


Kill this new line. We generally use `one-line` newlines to separate 
code-blocks.



src/tests/slave_tests.cpp (line 2190)


How about:

```
Port *port1 = ports.add_ports();
port1->set_number(80);
port1->mutable_labels()->CopyFrom(labels1);

Port *port2 = ports.add_ports();
port2->set_number(8081);
port2->mutable_labels()->CopyFrom(labels2);
```

This makes it a bit cleaner.



src/tests/slave_tests.cpp (line 2197)


Delete this comment.

Also PS, we start comments with one space and the first character should be 
`capital`

```
// Set DiscoveryInfo in the TaskInfo
```



src/tests/slave_tests.cpp (line 2202)


New line before.



src/tests/slave_tests.cpp (line 2204)


Kill this newline.



src/tests/slave_tests.cpp (line 2205)


Why not just do this:

```
EXPECT_CALL(exec, registered(_, _, _, _, _));
```

AFAICT, you don't seem to be using any of the arguments from the 
`registered` callback. All you are interested in is that the expectation is 
fullfilled exactly once.



src/tests/slave_tests.cpp (line 2209)


You don't seem to be using `execTask` i.e. `TaskInfo` attributed. Why not 
just do:

```
Future launchTask;
EXPECT_CALL(exec, launchTask(_, _))
  .WillOnce(FutureSatisfy(&launchTask));
```

and then later do

```AWAIT_READY(launchTask);```



src/tests/slave_tests.cpp (line 2220)


New line here.

We generally insert a newline after a statement spanning multiple lines.



src/tests/slave_tests.cpp (line )


We generally avoid magic string constants.

Can you just do

```
ASSERT_SOME_EQ(
APPLICATION_JSON,
response.get().headers.get("Content-Type"));
```

Can we also check if the response code is OK() ?



src/tests/slave_tests.cpp (line 2228)


Kill this comment.



src/tests/slave_tests.cpp (line 2234)


Kill this comment.

`EXPECT_EQ` is self explanatory.



src/tests/slave_tests.cpp (line 2238)


How about:

```
// Check that ports are set in the `DiscoveryInfo` object.
```



src/tests/slave_tests.cpp (line 2243)


Newline before.



src/tests/slave_tests.cpp (line 2245)


New line before this line.

Also,

`// Verify that the ports match.`



src/tests/slave_tests.cpp (line 2250)


Kill all the newlines. Just have one newline


- Anand Mazumdar


On Dec. 14, 2015, 11:34 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41188/
> ---
> 
> (Updated Dec. 14, 2015, 11:34 p.m.)
> 
> 
> Review request for mesos, Adam B and Anand Mazumdar.
> 
> 
> Bugs: MESOS-3962
> https://issues.apache.org/jira/browse/MESOS-3962
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Providing JSON bindings to so that mesos modules can expose DiscoveryInfo 
> protobuf messages to HTTP endpoints
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp d1b1158c0a80d72c32c9b28977043b6be2295239 
>   src

Re: Review Request 41188: Providing JSON bindings to so that mesos modules can expose DiscoveryInfo protobuf messages to HTTP endpoints

[Avinash sridharan]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41188/
---

(Updated Dec. 14, 2015, 11:34 p.m.)


Review request for mesos, Adam B and Anand Mazumdar.


Bugs: MESOS-3962
https://issues.apache.org/jira/browse/MESOS-3962


Repository: mesos


Description
---

Providing JSON bindings to so that mesos modules can expose DiscoveryInfo 
protobuf messages to HTTP endpoints


Diffs (updated)
-

  src/slave/http.cpp d1b1158c0a80d72c32c9b28977043b6be2295239 
  src/tests/common/http_tests.cpp 3aca5b0437a012664f58ff331cc7cf682d442699 
  src/tests/slave_tests.cpp 4975bea8a7a701e0414426760692720f73dea7f5 

Diff: https://reviews.apache.org/r/41188/diff/


Testing
---

make check.
Added Unit tests to verify setting of DiscoveryInfo in state.json in slave. 
Also added Unit test to test that DiscoveryInfo gets exposed in master when 
TaskInfo protobuf is converted to JSON objects.


Thanks,

Avinash sridharan

Re: Review Request 41188: Providing JSON bindings to so that mesos modules can expose DiscoveryInfo protobuf messages to HTTP endpoints

[Avinash sridharan]

> On Dec. 10, 2015, 4:52 a.m., Anand Mazumdar wrote:
> > 1. We badly need a unit test for this. :-)
> > 2. If I understand correctly, the reason you did not use the automated 
> > protobuf to JSON converters and had to use `model(...)` is that `labels` is 
> > populated in a custom way in the `state.json` endpoint and you wanted to 
> > preserve that ?

Added a unit-test case for testing DiscoveryInfo injection in state.json for 
slave and modeling of DiscoveryInfo as a JSON object in master. Also using the 
generic JSON::protobuf API to convert protobuf to JSON.


- Avinash


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41188/#review109684
---


On Dec. 12, 2015, 7:37 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41188/
> ---
> 
> (Updated Dec. 12, 2015, 7:37 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-3962
> https://issues.apache.org/jira/browse/MESOS-3962
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Providing JSON bindings to so that mesos modules can expose DiscoveryInfo 
> protobuf messages to HTTP endpoints
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp d1b1158c0a80d72c32c9b28977043b6be2295239 
>   src/tests/common/http_tests.cpp 3aca5b0437a012664f58ff331cc7cf682d442699 
>   src/tests/slave_tests.cpp 4975bea8a7a701e0414426760692720f73dea7f5 
> 
> Diff: https://reviews.apache.org/r/41188/diff/
> 
> 
> Testing
> ---
> 
> make check.
> Added Unit tests to verify setting of DiscoveryInfo in state.json in slave. 
> Also added Unit test to test that DiscoveryInfo gets exposed in master when 
> TaskInfo protobuf is converted to JSON objects.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 41188: Providing JSON bindings to so that mesos modules can expose DiscoveryInfo protobuf messages to HTTP endpoints

[Avinash sridharan]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41188/
---

(Updated Dec. 12, 2015, 7:37 a.m.)


Review request for mesos and Adam B.


Bugs: MESOS-3962
https://issues.apache.org/jira/browse/MESOS-3962


Repository: mesos


Description
---

Providing JSON bindings to so that mesos modules can expose DiscoveryInfo 
protobuf messages to HTTP endpoints


Diffs (updated)
-

  src/slave/http.cpp d1b1158c0a80d72c32c9b28977043b6be2295239 
  src/tests/common/http_tests.cpp 3aca5b0437a012664f58ff331cc7cf682d442699 
  src/tests/slave_tests.cpp 4975bea8a7a701e0414426760692720f73dea7f5 

Diff: https://reviews.apache.org/r/41188/diff/


Testing
---

make check.
Added Unit tests to verify setting of DiscoveryInfo in state.json in slave. 
Also added Unit test to test that DiscoveryInfo gets exposed in master when 
TaskInfo protobuf is converted to JSON objects.


Thanks,

Avinash sridharan

Re: Review Request 41188: Providing JSON bindings to so that mesos modules can expose DiscoveryInfo protobuf messages to HTTP endpoints

[Avinash sridharan]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41188/
---

(Updated Dec. 12, 2015, 6:34 a.m.)


Review request for mesos and Adam B.


Bugs: MESOS-3962
https://issues.apache.org/jira/browse/MESOS-3962


Repository: mesos


Description
---

Providing JSON bindings to so that mesos modules can expose DiscoveryInfo 
protobuf messages to HTTP endpoints


Diffs (updated)
-

  src/slave/http.cpp d1b1158c0a80d72c32c9b28977043b6be2295239 
  src/tests/common/http_tests.cpp 3aca5b0437a012664f58ff331cc7cf682d442699 
  src/tests/slave_tests.cpp 4975bea8a7a701e0414426760692720f73dea7f5 

Diff: https://reviews.apache.org/r/41188/diff/


Testing (updated)
---

make check.
Added Unit tests to verify setting of DiscoveryInfo in state.json in slave. 
Also added Unit test to test that DiscoveryInfo gets exposed in master when 
TaskInfo protobuf is converted to JSON objects.


Thanks,

Avinash sridharan

Re: Review Request 41188: Providing JSON bindings to so that mesos modules can expose DiscoveryInfo protobuf messages to HTTP endpoints

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41188/#review109752
---


Patch looks great!

Reviews applied: [41187, 41188]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 10, 2015, 3:36 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41188/
> ---
> 
> (Updated Dec. 10, 2015, 3:36 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-3962
> https://issues.apache.org/jira/browse/MESOS-3962
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Providing JSON bindings to so that mesos modules can expose DiscoveryInfo 
> protobuf messages to HTTP endpoints
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp 6a7a14ce8f1b9e0a85bc88bcf32bd80ea56bb7be 
>   src/common/http.cpp 5198650ba6dc1ea3dab7912e5ef6e375bf9acc96 
>   src/slave/http.cpp cef568d77da430b96e8f97487a8d0406dc0a0116 
> 
> Diff: https://reviews.apache.org/r/41188/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 41188: Providing JSON bindings to so that mesos modules can expose DiscoveryInfo protobuf messages to HTTP endpoints

[Anand Mazumdar]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41188/#review109684
---


1. We badly need a unit test for this. :-)
2. If I understand correctly, the reason you did not use the automated protobuf 
to JSON converters and had to use `model(...)` is that `labels` is populated in 
a custom way in the `state.json` endpoint and you wanted to preserve that ?


src/common/http.hpp (line 84)


Based on my comment in an earlier review , do we need this ?



src/common/http.cpp (line 158)


Missing period at the end.



src/common/http.cpp (line 159)


Do we need this ?



src/common/http.cpp (line 195)


Missing period at the end.



src/slave/http.cpp (line 96)


Missing Period


- Anand Mazumdar


On Dec. 10, 2015, 3:36 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41188/
> ---
> 
> (Updated Dec. 10, 2015, 3:36 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-3962
> https://issues.apache.org/jira/browse/MESOS-3962
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Providing JSON bindings to so that mesos modules can expose DiscoveryInfo 
> protobuf messages to HTTP endpoints
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp 6a7a14ce8f1b9e0a85bc88bcf32bd80ea56bb7be 
>   src/common/http.cpp 5198650ba6dc1ea3dab7912e5ef6e375bf9acc96 
>   src/slave/http.cpp cef568d77da430b96e8f97487a8d0406dc0a0116 
> 
> Diff: https://reviews.apache.org/r/41188/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 41188: Providing JSON bindings to so that mesos modules can expose DiscoveryInfo protobuf messages to HTTP endpoints

[Avinash sridharan]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41188/
---

(Updated Dec. 10, 2015, 3:36 a.m.)


Review request for mesos and Adam B.


Bugs: MESOS-3962
https://issues.apache.org/jira/browse/MESOS-3962


Repository: mesos


Description
---

Providing JSON bindings to so that mesos modules can expose DiscoveryInfo 
protobuf messages to HTTP endpoints


Diffs
-

  src/common/http.hpp 6a7a14ce8f1b9e0a85bc88bcf32bd80ea56bb7be 
  src/common/http.cpp 5198650ba6dc1ea3dab7912e5ef6e375bf9acc96 
  src/slave/http.cpp cef568d77da430b96e8f97487a8d0406dc0a0116 

Diff: https://reviews.apache.org/r/41188/diff/


Testing (updated)
---

make check

I haven't written explicit unit test cases for the JSON serialization for the 
new protobuf messages.


Thanks,

Avinash sridharan

Re: Review Request 41188: Providing JSON bindings to so that mesos modules can expose DiscoveryInfo protobuf messages to HTTP endpoints

[Avinash sridharan]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41188/
---

(Updated Dec. 10, 2015, 3:36 a.m.)


Review request for mesos and Adam B.


Bugs: MESOS-3962
https://issues.apache.org/jira/browse/MESOS-3962


Repository: mesos


Description
---

Providing JSON bindings to so that mesos modules can expose DiscoveryInfo 
protobuf messages to HTTP endpoints


Diffs
-

  src/common/http.hpp 6a7a14ce8f1b9e0a85bc88bcf32bd80ea56bb7be 
  src/common/http.cpp 5198650ba6dc1ea3dab7912e5ef6e375bf9acc96 
  src/slave/http.cpp cef568d77da430b96e8f97487a8d0406dc0a0116 

Diff: https://reviews.apache.org/r/41188/diff/


Testing (updated)
---

make check


Thanks,

Avinash sridharan