Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Niklas Nielsen]

> On June 10, 2015, 8:37 p.m., Ben Mahler wrote:
> > How about adding an 'Address' message, which can contain 'ip' and 'port' 
> > for now?
> > 
> > ```
> > message Address {
> >   required string ip;
> >   required uint32 port;
> >   
> >   // Later we can add 'hostname' or 'public_hostname', etc!
> > }
> > ```
> > 
> > In the future, we can further use this in other protobufs to have a 
> > conistent representation (e.g. SlaveInfo only has 'hostname' and 'port' 
> > currently, no 'ip'!). That way, you can add an address to MasterInfo:
> > 
> > ```
> > message MasterInfo {
> >   required string id = 1;
> >   required uint32 ip = 2;
> >   required uint32 port = 3 [default = 5050];
> >   optional string pid = 4;
> >   optional string hostname = 5;
> >   
> >   optional Address address = 6;
> > }
> > ```
> > 
> > This way, you don't need all the custom logic introduced here, and 
> > consequently compatibility is easier to test (i.e. we have already tested 
> > our JSON <-> Protobuf conversion facilities).
> > 
> > Have you guys considered this approach?

@BenH, Vinod and BenM: We have too many chefs her. Let's please assign one to 
drive this. It sounds like we have to take this back to the design (and JIRA 
discussions).

Here is my concern; I was assigned as shepherd on this and got blocked in the 
11th hour.
There is very little space to iterate on this and the patch would have worked 
fine. It was a part of a bigger patch set (zookeeper detection for HTTP, which 
Vinod drove) and I am amazed that a conversion between proto to json and 
vice-versa takes more than 2 weeks back and forth, to then (most likely) 
getting dropped. This is one example where both committers and contributors 
(us) have a bad experience. I would like to know how to get better and avoid 
this in the future, without giving up 'shepherd'ship.

@Marco; I am sorry that we wasted your (and our) time. Let's try to fix the 
process. It hurts me as well as I thought we had this in a good shape but it 
sounds to me like we need to drop this patch.


- Niklas


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


On June 5, 2015, 1:18 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated June 5, 2015, 1:18 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2807
> https://issues.apache.org/jira/browse/MESOS-2807
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f2110a360b6e2a1e99e1d9630d5dfacfd818530a 
>   src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 
>   src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
>   src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
>   src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
>   src/tests/common/parse_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Ben Mahler]

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


How about adding an 'Address' message, which can contain 'ip' and 'port' for 
now?

```
message Address {
  required string ip;
  required uint32 port;
  
  // Later we can add 'hostname' or 'public_hostname', etc!
}
```

In the future, we can further use this in other protobufs to have a conistent 
representation (e.g. SlaveInfo only has 'hostname' and 'port' currently, no 
'ip'!). That way, you can add an address to MasterInfo:

```
message MasterInfo {
  required string id = 1;
  required uint32 ip = 2;
  required uint32 port = 3 [default = 5050];
  optional string pid = 4;
  optional string hostname = 5;
  
  optional Address address = 6;
}
```

This way, you don't need all the custom logic introduced here, and consequently 
compatibility is easier to test (i.e. we have already tested our JSON <-> 
Protobuf conversion facilities).

Have you guys considered this approach?

- Ben Mahler


On June 5, 2015, 8:18 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated June 5, 2015, 8:18 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2807
> https://issues.apache.org/jira/browse/MESOS-2807
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f2110a360b6e2a1e99e1d9630d5dfacfd818530a 
>   src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 
>   src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
>   src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
>   src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
>   src/tests/common/parse_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Vinod Kone]

> On June 5, 2015, 6:42 p.m., Vinod Kone wrote:
> > I made some minor comments below but I think a better way to do this is to 
> > *not* write custom masterinfo json <-> protobuf converters. I prefer we 
> > just add a new optional field (say ipAddress of type string). Then you can 
> > just leverage the already existing generic protobuf <-> json converters.
> 
> Niklas Nielsen wrote:
> We have been working on this for a while now and we are using 
> JSON::Protobuf() aldready and can enhance it further with your suggestion. 
> However, the current approach isn't semantically different from the one you 
> suggest and we would like to move forward and make that change later. Is that 
> OK with you?
> 
> Marco Massenzio wrote:
> Hey Vinod - as Niklas pointed out, we have invested a significant amount 
> of time on this one, including the manual testing I've done (as summarized on 
> MESOS-2304) and I'd be really reluctant to start again from scratch.
> 
> As I don't really think there is any semantic difference; my approach 
> does not introduce any performance penalty (in fact, I believe it may even be 
> better than a 'generic' one); and, finally, that this does not impact the 
> readability of the code, we should commit the code 'as is' (with your 
> suggestions below) and move on.
> 
> There are a ton of features and work to do, and I'd like us to focus on 
> important issues.
> 
> Vinod Kone wrote:
> Marco. I appreciate that you invested significant effort to making sure 
> the backwards compatibility story is airtight, but I urge you to spend some 
> extra cycles to simplify the code and avoid tech debt. I honestly don't think 
> it would take too much time to simplify this. I would really like to 
> understand what's the most time consuming part here? The compatibility 
> testing? Can you automate it with niklas's compatibility script?
> 
> As an aside, going through earlier reviews I saw that BenH made similar 
> comments but it got sidetracked with deprecating "ip". Also, my fault for not 
> jumping on this sooner, but in my defense, I wasn't marked as a reviewer. I 
> would be happy to shepherd this change for you going forward.
> 
> Marco Massenzio wrote:
> The upgrade/change of MasterInfo is tracked in 
> https://issues.apache.org/jira/browse/MESOS-2736
> which I believe is a different topic than what is addressed here (which 
> is tracked here: https://issues.apache.org/jira/browse/MESOS-2807 - I'll 
> update this patch description in a sec).
> 
> In any event, as `ip` is a required int32 field, we MUST support it, no 
> matter what - this patch does this, correctly, without introducing "tech 
> debt" (I don't see where I'm doing that).
> 
> Thanks in advance for your understanding.
> 
> Vinod Kone wrote:
> My suggestion is to support current ip field by keeping it asis, i.e., 
> let it be a number in json string. The reasoning is that, if we end up 
> deciding that the easiest way to support ipv6 is by having a string field in 
> the protobuf, then we would've 2 redundant string representations of the ip 
> address in the resulting json.
> 
> The tech debt part i was referring to was that, now there is are a set of 
> custom functions for protobuf <-> json conversions for this protobuf which 
> need to be maintained (while model() is not bad, parse() is doing a lot of 
> work and seems to have written without the knowledge that there is an 
> existing generic parse function IIRC?). For example, if someone adds new 
> fields to MasterInfo they have to come and update these functions. Not to 
> mention, you have added a bunch of tests to test the custom parsing logic, 
> which could likely be eliminated if you can reuse the existing generic 
> functions.
> 
> Feel free to ping me on IRC if you want to discuss further.
> 
> Benjamin Hindman wrote:
> Folks, let's leverage what we have here as it's pretty clear from this 
> discussion that we haven't figured out the answer yet for deprecating the 
> 'ip' field and adding an IPv6 supported 'address' field or something similar. 
> It's not worth rewriting this from scratch so instead let's do the next best 
> thing (as we have other places too) and leave some great comments and TODOs 
> around why the current approach was taken and what are the next steps to fix 
> it. I think we're all on the same page that not having to write our own 
> 'model' and 'parse' functions or have the JSON types differ from the protobuf 
> types is the preferred approach, but given we're going to be deprecating the 
> 'ip' field all together we can simply remove it completely (after making it 
> optional) while simultaneously introducing the new field. It'll mean we have 
> to keep the custom 'model' and 'parse' functions around a bit longer (while 
> the 'ip' field still exists), but provided they're clearly documented this 
> should b
 e very minor and manageable. Let's keep the cadence

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Benjamin Hindman]

> On June 5, 2015, 6:42 p.m., Vinod Kone wrote:
> > I made some minor comments below but I think a better way to do this is to 
> > *not* write custom masterinfo json <-> protobuf converters. I prefer we 
> > just add a new optional field (say ipAddress of type string). Then you can 
> > just leverage the already existing generic protobuf <-> json converters.
> 
> Niklas Nielsen wrote:
> We have been working on this for a while now and we are using 
> JSON::Protobuf() aldready and can enhance it further with your suggestion. 
> However, the current approach isn't semantically different from the one you 
> suggest and we would like to move forward and make that change later. Is that 
> OK with you?
> 
> Marco Massenzio wrote:
> Hey Vinod - as Niklas pointed out, we have invested a significant amount 
> of time on this one, including the manual testing I've done (as summarized on 
> MESOS-2304) and I'd be really reluctant to start again from scratch.
> 
> As I don't really think there is any semantic difference; my approach 
> does not introduce any performance penalty (in fact, I believe it may even be 
> better than a 'generic' one); and, finally, that this does not impact the 
> readability of the code, we should commit the code 'as is' (with your 
> suggestions below) and move on.
> 
> There are a ton of features and work to do, and I'd like us to focus on 
> important issues.
> 
> Vinod Kone wrote:
> Marco. I appreciate that you invested significant effort to making sure 
> the backwards compatibility story is airtight, but I urge you to spend some 
> extra cycles to simplify the code and avoid tech debt. I honestly don't think 
> it would take too much time to simplify this. I would really like to 
> understand what's the most time consuming part here? The compatibility 
> testing? Can you automate it with niklas's compatibility script?
> 
> As an aside, going through earlier reviews I saw that BenH made similar 
> comments but it got sidetracked with deprecating "ip". Also, my fault for not 
> jumping on this sooner, but in my defense, I wasn't marked as a reviewer. I 
> would be happy to shepherd this change for you going forward.
> 
> Marco Massenzio wrote:
> The upgrade/change of MasterInfo is tracked in 
> https://issues.apache.org/jira/browse/MESOS-2736
> which I believe is a different topic than what is addressed here (which 
> is tracked here: https://issues.apache.org/jira/browse/MESOS-2807 - I'll 
> update this patch description in a sec).
> 
> In any event, as `ip` is a required int32 field, we MUST support it, no 
> matter what - this patch does this, correctly, without introducing "tech 
> debt" (I don't see where I'm doing that).
> 
> Thanks in advance for your understanding.
> 
> Vinod Kone wrote:
> My suggestion is to support current ip field by keeping it asis, i.e., 
> let it be a number in json string. The reasoning is that, if we end up 
> deciding that the easiest way to support ipv6 is by having a string field in 
> the protobuf, then we would've 2 redundant string representations of the ip 
> address in the resulting json.
> 
> The tech debt part i was referring to was that, now there is are a set of 
> custom functions for protobuf <-> json conversions for this protobuf which 
> need to be maintained (while model() is not bad, parse() is doing a lot of 
> work and seems to have written without the knowledge that there is an 
> existing generic parse function IIRC?). For example, if someone adds new 
> fields to MasterInfo they have to come and update these functions. Not to 
> mention, you have added a bunch of tests to test the custom parsing logic, 
> which could likely be eliminated if you can reuse the existing generic 
> functions.
> 
> Feel free to ping me on IRC if you want to discuss further.

Folks, let's leverage what we have here as it's pretty clear from this 
discussion that we haven't figured out the answer yet for deprecating the 'ip' 
field and adding an IPv6 supported 'address' field or something similar. It's 
not worth rewriting this from scratch so instead let's do the next best thing 
(as we have other places too) and leave some great comments and TODOs around 
why the current approach was taken and what are the next steps to fix it. I 
think we're all on the same page that not having to write our own 'model' and 
'parse' functions or have the JSON types differ from the protobuf types is the 
preferred approach, but given we're going to be deprecating the 'ip' field all 
together we can simply remove it completely (after making it optional) while 
simultaneously introducing the new field. It'll mean we have to keep the custom 
'model' and 'parse' functions around a bit longer (while the 'ip' field still 
exists), but provided they're clearly documented this should be very
  minor and manageable. Let's keep the cadence going please!


- Benjamin


---

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Vinod Kone]

> On June 5, 2015, 6:42 p.m., Vinod Kone wrote:
> > I made some minor comments below but I think a better way to do this is to 
> > *not* write custom masterinfo json <-> protobuf converters. I prefer we 
> > just add a new optional field (say ipAddress of type string). Then you can 
> > just leverage the already existing generic protobuf <-> json converters.
> 
> Niklas Nielsen wrote:
> We have been working on this for a while now and we are using 
> JSON::Protobuf() aldready and can enhance it further with your suggestion. 
> However, the current approach isn't semantically different from the one you 
> suggest and we would like to move forward and make that change later. Is that 
> OK with you?
> 
> Marco Massenzio wrote:
> Hey Vinod - as Niklas pointed out, we have invested a significant amount 
> of time on this one, including the manual testing I've done (as summarized on 
> MESOS-2304) and I'd be really reluctant to start again from scratch.
> 
> As I don't really think there is any semantic difference; my approach 
> does not introduce any performance penalty (in fact, I believe it may even be 
> better than a 'generic' one); and, finally, that this does not impact the 
> readability of the code, we should commit the code 'as is' (with your 
> suggestions below) and move on.
> 
> There are a ton of features and work to do, and I'd like us to focus on 
> important issues.
> 
> Vinod Kone wrote:
> Marco. I appreciate that you invested significant effort to making sure 
> the backwards compatibility story is airtight, but I urge you to spend some 
> extra cycles to simplify the code and avoid tech debt. I honestly don't think 
> it would take too much time to simplify this. I would really like to 
> understand what's the most time consuming part here? The compatibility 
> testing? Can you automate it with niklas's compatibility script?
> 
> As an aside, going through earlier reviews I saw that BenH made similar 
> comments but it got sidetracked with deprecating "ip". Also, my fault for not 
> jumping on this sooner, but in my defense, I wasn't marked as a reviewer. I 
> would be happy to shepherd this change for you going forward.
> 
> Marco Massenzio wrote:
> The upgrade/change of MasterInfo is tracked in 
> https://issues.apache.org/jira/browse/MESOS-2736
> which I believe is a different topic than what is addressed here (which 
> is tracked here: https://issues.apache.org/jira/browse/MESOS-2807 - I'll 
> update this patch description in a sec).
> 
> In any event, as `ip` is a required int32 field, we MUST support it, no 
> matter what - this patch does this, correctly, without introducing "tech 
> debt" (I don't see where I'm doing that).
> 
> Thanks in advance for your understanding.

My suggestion is to support current ip field by keeping it asis, i.e., let it 
be a number in json string. The reasoning is that, if we end up deciding that 
the easiest way to support ipv6 is by having a string field in the protobuf, 
then we would've 2 redundant string representations of the ip address in the 
resulting json.

The tech debt part i was referring to was that, now there is are a set of 
custom functions for protobuf <-> json conversions for this protobuf which need 
to be maintained (while model() is not bad, parse() is doing a lot of work and 
seems to have written without the knowledge that there is an existing generic 
parse function IIRC?). For example, if someone adds new fields to MasterInfo 
they have to come and update these functions. Not to mention, you have added a 
bunch of tests to test the custom parsing logic, which could likely be 
eliminated if you can reuse the existing generic functions.

Feel free to ping me on IRC if you want to discuss further.


- Vinod


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


On June 5, 2015, 8:18 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated June 5, 2015, 8:18 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2807
> https://issues.apache.org/jira/browse/MESOS-2807
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associa

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Marco Massenzio]

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

(Updated June 5, 2015, 8:18 p.m.)


Review request for mesos, haosdent huang and Niklas Nielsen.


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


Repository: mesos


Description
---

Jira: MESOS-2340

This is a preliminary step to enabling JSON API
for Master Discovery via Zookeeper.

We currently save the MasterInfo PB to ZK
serializing directly the binary data, so that
for clients to retrieve that information, they
need to either link up with libmesos or
obtain the PB definition (in mesos/mesos.proto).

This change only provides the (de)serialization
utility methods and associated tests.


Diffs
-

  src/Makefile.am f2110a360b6e2a1e99e1d9630d5dfacfd818530a 
  src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 
  src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
  src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
  src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
  src/tests/common/parse_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Marco Massenzio

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Marco Massenzio]

> On June 5, 2015, 6:42 p.m., Vinod Kone wrote:
> > I made some minor comments below but I think a better way to do this is to 
> > *not* write custom masterinfo json <-> protobuf converters. I prefer we 
> > just add a new optional field (say ipAddress of type string). Then you can 
> > just leverage the already existing generic protobuf <-> json converters.
> 
> Niklas Nielsen wrote:
> We have been working on this for a while now and we are using 
> JSON::Protobuf() aldready and can enhance it further with your suggestion. 
> However, the current approach isn't semantically different from the one you 
> suggest and we would like to move forward and make that change later. Is that 
> OK with you?
> 
> Marco Massenzio wrote:
> Hey Vinod - as Niklas pointed out, we have invested a significant amount 
> of time on this one, including the manual testing I've done (as summarized on 
> MESOS-2304) and I'd be really reluctant to start again from scratch.
> 
> As I don't really think there is any semantic difference; my approach 
> does not introduce any performance penalty (in fact, I believe it may even be 
> better than a 'generic' one); and, finally, that this does not impact the 
> readability of the code, we should commit the code 'as is' (with your 
> suggestions below) and move on.
> 
> There are a ton of features and work to do, and I'd like us to focus on 
> important issues.
> 
> Vinod Kone wrote:
> Marco. I appreciate that you invested significant effort to making sure 
> the backwards compatibility story is airtight, but I urge you to spend some 
> extra cycles to simplify the code and avoid tech debt. I honestly don't think 
> it would take too much time to simplify this. I would really like to 
> understand what's the most time consuming part here? The compatibility 
> testing? Can you automate it with niklas's compatibility script?
> 
> As an aside, going through earlier reviews I saw that BenH made similar 
> comments but it got sidetracked with deprecating "ip". Also, my fault for not 
> jumping on this sooner, but in my defense, I wasn't marked as a reviewer. I 
> would be happy to shepherd this change for you going forward.

The upgrade/change of MasterInfo is tracked in 
https://issues.apache.org/jira/browse/MESOS-2736
which I believe is a different topic than what is addressed here (which is 
tracked here: https://issues.apache.org/jira/browse/MESOS-2807 - I'll update 
this patch description in a sec).

In any event, as `ip` is a required int32 field, we MUST support it, no matter 
what - this patch does this, correctly, without introducing "tech debt" (I 
don't see where I'm doing that).

Thanks in advance for your understanding.


- Marco


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


On June 3, 2015, 9 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated June 3, 2015, 9 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2340
> https://issues.apache.org/jira/browse/MESOS-2340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f2110a360b6e2a1e99e1d9630d5dfacfd818530a 
>   src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 
>   src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
>   src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
>   src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
>   src/tests/common/parse_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Vinod Kone]

> On June 5, 2015, 6:42 p.m., Vinod Kone wrote:
> > I made some minor comments below but I think a better way to do this is to 
> > *not* write custom masterinfo json <-> protobuf converters. I prefer we 
> > just add a new optional field (say ipAddress of type string). Then you can 
> > just leverage the already existing generic protobuf <-> json converters.
> 
> Niklas Nielsen wrote:
> We have been working on this for a while now and we are using 
> JSON::Protobuf() aldready and can enhance it further with your suggestion. 
> However, the current approach isn't semantically different from the one you 
> suggest and we would like to move forward and make that change later. Is that 
> OK with you?
> 
> Marco Massenzio wrote:
> Hey Vinod - as Niklas pointed out, we have invested a significant amount 
> of time on this one, including the manual testing I've done (as summarized on 
> MESOS-2304) and I'd be really reluctant to start again from scratch.
> 
> As I don't really think there is any semantic difference; my approach 
> does not introduce any performance penalty (in fact, I believe it may even be 
> better than a 'generic' one); and, finally, that this does not impact the 
> readability of the code, we should commit the code 'as is' (with your 
> suggestions below) and move on.
> 
> There are a ton of features and work to do, and I'd like us to focus on 
> important issues.

Marco. I appreciate that you invested significant effort to making sure the 
backwards compatibility story is airtight, but I urge you to spend some extra 
cycles to simplify the code and avoid tech debt. I honestly don't think it 
would take too much time to simplify this. I would really like to understand 
what's the most time consuming part here? The compatibility testing? Can you 
automate it with niklas's compatibility script?

As an aside, going through earlier reviews I saw that BenH made similar 
comments but it got sidetracked with deprecating "ip". Also, my fault for not 
jumping on this sooner, but in my defense, I wasn't marked as a reviewer. I 
would be happy to shepherd this change for you going forward.


- Vinod


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


On June 3, 2015, 9 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated June 3, 2015, 9 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2340
> https://issues.apache.org/jira/browse/MESOS-2340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f2110a360b6e2a1e99e1d9630d5dfacfd818530a 
>   src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 
>   src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
>   src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
>   src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
>   src/tests/common/parse_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Marco Massenzio]

> On June 5, 2015, 6:42 p.m., Vinod Kone wrote:
> > I made some minor comments below but I think a better way to do this is to 
> > *not* write custom masterinfo json <-> protobuf converters. I prefer we 
> > just add a new optional field (say ipAddress of type string). Then you can 
> > just leverage the already existing generic protobuf <-> json converters.
> 
> Niklas Nielsen wrote:
> We have been working on this for a while now and we are using 
> JSON::Protobuf() aldready and can enhance it further with your suggestion. 
> However, the current approach isn't semantically different from the one you 
> suggest and we would like to move forward and make that change later. Is that 
> OK with you?

Hey Vinod - as Niklas pointed out, we have invested a significant amount of 
time on this one, including the manual testing I've done (as summarized on 
MESOS-2304) and I'd be really reluctant to start again from scratch.

As I don't really think there is any semantic difference; my approach does not 
introduce any performance penalty (in fact, I believe it may even be better 
than a 'generic' one); and, finally, that this does not impact the readability 
of the code, we should commit the code 'as is' (with your suggestions below) 
and move on.

There are a ton of features and work to do, and I'd like us to focus on 
important issues.


- Marco


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


On June 3, 2015, 9 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated June 3, 2015, 9 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2340
> https://issues.apache.org/jira/browse/MESOS-2340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f2110a360b6e2a1e99e1d9630d5dfacfd818530a 
>   src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 
>   src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
>   src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
>   src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
>   src/tests/common/parse_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Niklas Nielsen]

> On June 5, 2015, 11:42 a.m., Vinod Kone wrote:
> > I made some minor comments below but I think a better way to do this is to 
> > *not* write custom masterinfo json <-> protobuf converters. I prefer we 
> > just add a new optional field (say ipAddress of type string). Then you can 
> > just leverage the already existing generic protobuf <-> json converters.

We have been working on this for a while now and we are using JSON::Protobuf() 
aldready and can enhance it further with your suggestion. However, the current 
approach isn't semantically different from the one you suggest and we would 
like to move forward and make that change later. Is that OK with you?


- Niklas


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


On June 3, 2015, 2 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated June 3, 2015, 2 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2340
> https://issues.apache.org/jira/browse/MESOS-2340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f2110a360b6e2a1e99e1d9630d5dfacfd818530a 
>   src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 
>   src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
>   src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
>   src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
>   src/tests/common/parse_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Niklas Nielsen]

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



src/common/http.hpp


s/info/masterInfo/



src/common/http.cpp


Unfold to:

```
/**
 * Foobar
 */
```



src/common/parse.hpp


Should this block have 2 space indent or 1? You have 1 above :)



src/tests/common/parse_tests.cpp


ASSERT_EQ() on the master infos instead of SerializeAsString()?


- Niklas Nielsen


On June 3, 2015, 2 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated June 3, 2015, 2 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2340
> https://issues.apache.org/jira/browse/MESOS-2340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f2110a360b6e2a1e99e1d9630d5dfacfd818530a 
>   src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 
>   src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
>   src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
>   src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
>   src/tests/common/parse_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Niklas Nielsen]

> On June 5, 2015, 11:42 a.m., Vinod Kone wrote:
> > src/common/http.cpp, line 212
> > 
> >
> > Why do you need this temporary?

BenH brought this up - look above. I think this is fine if it is made const.


- Niklas


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


On June 3, 2015, 2 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated June 3, 2015, 2 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2340
> https://issues.apache.org/jira/browse/MESOS-2340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f2110a360b6e2a1e99e1d9630d5dfacfd818530a 
>   src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 
>   src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
>   src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
>   src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
>   src/tests/common/parse_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Vinod Kone]

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


I made some minor comments below but I think a better way to do this is to 
*not* write custom masterinfo json <-> protobuf converters. I prefer we just 
add a new optional field (say ipAddress of type string). Then you can just 
leverage the already existing generic protobuf <-> json converters.


src/common/http.cpp


Why do you need this temporary?



src/common/parse.hpp


s/error out/return an error/



src/common/parse.hpp


s/result/info/



src/common/parse.hpp


"ips" sounds like multiple IPs though i know you meant it as IP as a string.

just call it "ipString"



src/common/parse.hpp


s/this/This/


- Vinod Kone


On June 3, 2015, 9 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated June 3, 2015, 9 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2340
> https://issues.apache.org/jira/browse/MESOS-2340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f2110a360b6e2a1e99e1d9630d5dfacfd818530a 
>   src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 
>   src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
>   src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
>   src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
>   src/tests/common/parse_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [34687]

All tests passed.

- Mesos ReviewBot


On June 3, 2015, 9 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated June 3, 2015, 9 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2340
> https://issues.apache.org/jira/browse/MESOS-2340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f2110a360b6e2a1e99e1d9630d5dfacfd818530a 
>   src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 
>   src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
>   src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
>   src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
>   src/tests/common/parse_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Marco Massenzio]

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

(Updated June 3, 2015, 9 p.m.)


Review request for mesos, haosdent huang and Niklas Nielsen.


Changes
---

nnielsen comments


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


Repository: mesos


Description
---

Jira: MESOS-2340

This is a preliminary step to enabling JSON API
for Master Discovery via Zookeeper.

We currently save the MasterInfo PB to ZK
serializing directly the binary data, so that
for clients to retrieve that information, they
need to either link up with libmesos or
obtain the PB definition (in mesos/mesos.proto).

This change only provides the (de)serialization
utility methods and associated tests.


Diffs (updated)
-

  src/Makefile.am f2110a360b6e2a1e99e1d9630d5dfacfd818530a 
  src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 
  src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
  src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
  src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
  src/tests/common/parse_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Marco Massenzio

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Marco Massenzio]

> On June 3, 2015, 6:49 p.m., Niklas Nielsen wrote:
> > src/common/http.cpp, line 209
> > 
> >
> > We haven't established a doxygen style in the style guide yet, but 
> > shouldn't we expand this to:
> > 
> > /**
> >  *  Foo Bar Baz
> >  */
> > 
> > ?
> 
> Niklas Nielsen wrote:
> Gawk - review board didn't catch my newlines:
> 
> ```
> /**
>  * Foo bar baz
>  */
> ```
> 
> was my intent.

you can have the "brief format" for short comments


> On June 3, 2015, 6:49 p.m., Niklas Nielsen wrote:
> > src/common/parse.hpp, line 178
> > 
> >
> > What if in() is an error? Shouldn't we check for that too?

no - `parse()` would have resulted in an error (see L170)
it is non-intuitive, that's why I defined this code "gnarly": one has to dip 
one's hand deep in the cookie jar to retrieve what one's lookign for :)


> On June 3, 2015, 6:49 p.m., Niklas Nielsen wrote:
> > src/tests/common/http_tests.cpp, line 121
> > 
> >
> > Mind adding a test description?

No, I don't mind at all - but why here? it's not done anywhere else, and it's a 
pretty straightforward test.
Done.


> On June 3, 2015, 6:49 p.m., Niklas Nielsen wrote:
> > src/tests/common/http_tests.cpp, line 129
> > 
> >
> > Do you want ASSERT_SOME(ip.in());?

Done - but it's largely redundant: we DO know the IP address is an invariant 
(const string) and well-formed: the only way this breaks is if we introduce a 
bug in net::IP::parse() - but that should be caught by **those** unit tests, 
now, shouldn't it? ;)


> On June 3, 2015, 6:49 p.m., Niklas Nielsen wrote:
> > src/tests/common/http_tests.cpp, lines 139-144
> > 
> >
> > As jsonMasterInfo will be a json object, how about doing equality 
> > between json objects instead of stringifying?
> > 
> > auto expected = JSON::parse("{ hostname ...");
> > 
> > ASSERT_EQ(expected.get(), jsonMasterInfo.get());
> > 
> > That would be robust to any spaces,newlines in the inlined JSON.
> > 
> > If you like it, we can do it here and below.

done - btw, that code would not compile; but a variation thereof did, which is 
great!
(I did try that before but could not get it to compile, so eventually gave up - 
other JSON-related tests in the codebase do use `stringify` instead)


> On June 3, 2015, 6:49 p.m., Niklas Nielsen wrote:
> > src/tests/common/http_tests.cpp, line 161
> > 
> >
> > End with period.

hey, at least I'm consistently wrong! :)


> On June 3, 2015, 6:49 p.m., Niklas Nielsen wrote:
> > src/tests/common/http_tests.cpp, line 162
> > 
> >
> > Need an ASSERT_SOME() on jsonMasterInfo.get().find("id") ?

I don't think so (but happy to be convinced otherwise): `id` is a required 
field, so if not present, it will cause the `model()` above to error out, and 
that will be caught by the `ASSERT_SOME(jsonMasterInfo)` above.
The only way this fails if we, due to a refactoring, "forget" to add back the 
"id" in the JSON - in which case, the breakage is A Good Thing, maybe?


> On June 3, 2015, 6:49 p.m., Niklas Nielsen wrote:
> > src/tests/common/parse_tests.cpp, line 19
> > 
> >
> > Shouldn't this include be located near line 35?

not according to Google (and cpplint): this is the "related header" that should 
precede every other headers (system, C++, etc.)
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Names_and_Order_of_Includes


> On June 3, 2015, 6:49 p.m., Niklas Nielsen wrote:
> > src/tests/common/parse_tests.cpp, line 47
> > 
> >
> > Test description :)

done (everywhere, actually).
Out of curiousity: is this a general requirement?
I haven't found anything in the style guide (Mesos and/or Google) that says 
anything about mandatory comments for TEST()s (in fact, I'm positive we did not 
required that at G).

(if so, however, it would have been nice to know during the first round ;)


> On June 3, 2015, 6:49 p.m., Niklas Nielsen wrote:
> > src/tests/common/parse_tests.cpp, lines 142-145
> > 
> >
> > Just tried this out: include  and you should have 
> > the == operator overload, be able to strip the '.SerializeAsString()' and 
> > kill the comment :)

yay! thanks :)


- Marco


---
This 

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Niklas Nielsen]

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


Looking much better! :)

A few suggestions below and let's get this in.


src/common/http.cpp


We haven't established a doxygen style in the style guide yet, but 
shouldn't we expand this to:

/**
 *  Foo Bar Baz
 */

?



src/common/http.cpp


s/`/'/
Error messages don't end with period (see 
http://mesos.apache.org/documentation/latest/mesos-c++-style-guide/)



src/common/parse.hpp


Move this to 'newline' - see: 
https://github.com/apache/mesos/blob/master/3rdparty/libprocess/include/process/clock.hpp#L28



src/common/parse.hpp


s/`/'/ and end with period



src/common/parse.hpp


What if in() is an error? Shouldn't we check for that too?



src/tests/common/http_tests.cpp


Mind adding a test description?



src/tests/common/http_tests.cpp


Do you want ASSERT_SOME(ip.in());?



src/tests/common/http_tests.cpp


As jsonMasterInfo will be a json object, how about doing equality between 
json objects instead of stringifying?

auto expected = JSON::parse("{ hostname ...");

ASSERT_EQ(expected.get(), jsonMasterInfo.get());

That would be robust to any spaces,newlines in the inlined JSON.

If you like it, we can do it here and below.



src/tests/common/http_tests.cpp


Test description :)



src/tests/common/http_tests.cpp


End with period?



src/tests/common/http_tests.cpp


s/the/The/



src/tests/common/http_tests.cpp


End with period.



src/tests/common/http_tests.cpp


Need an ASSERT_SOME() on jsonMasterInfo.get().find("id") ?



src/tests/common/parse_tests.cpp


Shouldn't this include be located near line 35?



src/tests/common/parse_tests.cpp


Test description :)



src/tests/common/parse_tests.cpp


s/yields back/yields/?



src/tests/common/parse_tests.cpp


MInfo? Should it be jsonMasterInfo?



src/tests/common/parse_tests.cpp


Just tried this out: include  and you should have the 
== operator overload, be able to strip the '.SerializeAsString()' and kill the 
comment :)


- Niklas Nielsen


On June 2, 2015, 2:52 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated June 2, 2015, 2:52 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2340
> https://issues.apache.org/jira/browse/MESOS-2340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f2110a360b6e2a1e99e1d9630d5dfacfd818530a 
>   src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 
>   src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
>   src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
>   src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
>   src/tests/common/parse_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Niklas Nielsen]

> On June 3, 2015, 11:49 a.m., Niklas Nielsen wrote:
> > src/common/http.cpp, line 209
> > 
> >
> > We haven't established a doxygen style in the style guide yet, but 
> > shouldn't we expand this to:
> > 
> > /**
> >  *  Foo Bar Baz
> >  */
> > 
> > ?

Gawk - review board didn't catch my newlines:

```
/**
 * Foo bar baz
 */
```

was my intent.


- Niklas


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


On June 2, 2015, 2:52 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated June 2, 2015, 2:52 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2340
> https://issues.apache.org/jira/browse/MESOS-2340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f2110a360b6e2a1e99e1d9630d5dfacfd818530a 
>   src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 
>   src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
>   src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
>   src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
>   src/tests/common/parse_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [34687]

All tests passed.

- Mesos ReviewBot


On June 2, 2015, 9:52 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated June 2, 2015, 9:52 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2340
> https://issues.apache.org/jira/browse/MESOS-2340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f2110a360b6e2a1e99e1d9630d5dfacfd818530a 
>   src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 
>   src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
>   src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
>   src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
>   src/tests/common/parse_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Marco Massenzio]

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

(Updated June 2, 2015, 9:52 p.m.)


Review request for mesos, haosdent huang and Niklas Nielsen.


Changes
---

BenH's comments


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


Repository: mesos


Description
---

Jira: MESOS-2340

This is a preliminary step to enabling JSON API
for Master Discovery via Zookeeper.

We currently save the MasterInfo PB to ZK
serializing directly the binary data, so that
for clients to retrieve that information, they
need to either link up with libmesos or
obtain the PB definition (in mesos/mesos.proto).

This change only provides the (de)serialization
utility methods and associated tests.


Diffs (updated)
-

  src/Makefile.am f2110a360b6e2a1e99e1d9630d5dfacfd818530a 
  src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 
  src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
  src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
  src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
  src/tests/common/parse_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Marco Massenzio

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Marco Massenzio]

> On June 2, 2015, 9:10 a.m., Benjamin Hindman wrote:
> > src/common/http.cpp, lines 212-213
> > 
> >
> > Please see formatting for braces: 
> > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Braced_Initializer_List_Format
> > 
> > tl;dr; Use braced-initializer list construction like a constructor.
> > 
> > JSON::Protobuf protobuf{masterInfo};
> > 
> > Please fix throughout this review.
> > 
> > But this brings me to a second question, why use braced-initializer 
> > list here at all when you're just calling a normal constructor (i.e., not a 
> > constructor that takes an std::initializer_list or an implicit constructor 
> > that just sets the fields of the object).
> > 
> > Finally, this can be simplified:
> > 
> > JSON::Object json(JSON::Protobuf(masterInfo));
> 
> Marco Massenzio wrote:
> sorry - this was a remnant from some trial code I'd run when playing 
> around with JSON::Protobuf (just to see what came out).
> Fixed.
> 
> I'm hoping I can bring cpplint to detect all this stuff.

sorry to break your heart, mate - but your code causes the dreaded "most vexing 
parse"... other variants fail too.
I'm sure we can come with a clever one-liner constructor, but in the spirit of 
the C++ Committee 
(guidelines](https://isocpp.org/std/library-design-guidelines) of "don't be 
clever", would you object to a (readable):
```
JSON::Protobuf protobuf(masterInfo);
JSON::Object json(protobuf);
```
not too "clever", but not terribly "stupid" either :)


- Marco


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


On May 31, 2015, 2:58 a.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated May 31, 2015, 2:58 a.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2340
> https://issues.apache.org/jira/browse/MESOS-2340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e7281ac6667562a27b0427b0ba7f41de98702928 
>   src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 
>   src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
>   src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
>   src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
>   src/tests/common/parse_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Marco Massenzio]

> On June 2, 2015, 9:10 a.m., Benjamin Hindman wrote:
> > I haven't done a full review here but took interest in this because the 
> > required 'ip' field of the MasterInfo protobuf (as well as other protobufs) 
> > is deprecated because it doesn't support IPv6. We need to make 'ip' an 
> > optional field and replace it by an 'address' field which includes both the 
> > 'hostname' or 'ip' and the port that can be parsed by our Address. Once we 
> > do that, then we shoudn't need any special JSON conversion or parsing.

Well, according to Protobuf Law, you can't: `required` is like a diamond: it's 
for life! ;)

But I'm all for it - if you think this is safe to do over a couple of releases?

I had already created https://issues.apache.org/jira/browse/MESOS-2736 - please 
add your suggestions/comments there.

Once we finalize that, I think patching this code to make it work with the new 
design should be trivial.


> On June 2, 2015, 9:10 a.m., Benjamin Hindman wrote:
> > src/common/http.hpp, line 52
> > 
> >
> > Please end sentences with a period.

Just so you know, in my spare time I'm submitting patches to cpplint.py :-)
(thanks for catching it!)


> On June 2, 2015, 9:10 a.m., Benjamin Hindman wrote:
> > src/common/http.cpp, lines 212-213
> > 
> >
> > Please see formatting for braces: 
> > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Braced_Initializer_List_Format
> > 
> > tl;dr; Use braced-initializer list construction like a constructor.
> > 
> > JSON::Protobuf protobuf{masterInfo};
> > 
> > Please fix throughout this review.
> > 
> > But this brings me to a second question, why use braced-initializer 
> > list here at all when you're just calling a normal constructor (i.e., not a 
> > constructor that takes an std::initializer_list or an implicit constructor 
> > that just sets the fields of the object).
> > 
> > Finally, this can be simplified:
> > 
> > JSON::Object json(JSON::Protobuf(masterInfo));

sorry - this was a remnant from some trial code I'd run when playing around 
with JSON::Protobuf (just to see what came out).
Fixed.

I'm hoping I can bring cpplint to detect all this stuff.


> On June 2, 2015, 9:10 a.m., Benjamin Hindman wrote:
> > src/common/http.cpp, line 219
> > 
> >
> > This seems rather aggressive, while I understand that 'ip' is required, 
> > that doesn't mean that someone might not pass an incomplete protobuf where 
> > this could crash hard instead of just returning an Error.

Well, I'd asked around whether there was a method to check both !error && !none 
and CHECK_SOME() was what had come up.
My bad.


> On June 2, 2015, 9:10 a.m., Benjamin Hindman wrote:
> > src/common/http.cpp, lines 220-225
> > 
> >
> > Why the extra temporary? Doing the compilers job Marco? ;-)
> > 
> > net::IP ip(ntohl(ipAsInt.get().value));

old habit - when I see gnarly code, my brain immediately projects an image of 
me doing debugging :)
hey, compared to a manager's job, a compiler's is a piece of cake!


- Marco


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


On May 31, 2015, 2:58 a.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated May 31, 2015, 2:58 a.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2340
> https://issues.apache.org/jira/browse/MESOS-2340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e7281ac6667562a27b0427b0ba7f41de98702928 
>   src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 
>   src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
>   src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
>   src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
>   src/tests/common/parse_tests.cpp PRE-CREATION 
> 
> Diff: https://revie

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Benjamin Hindman]

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


I haven't done a full review here but took interest in this because the 
required 'ip' field of the MasterInfo protobuf (as well as other protobufs) is 
deprecated because it doesn't support IPv6. We need to make 'ip' an optional 
field and replace it by an 'address' field which includes both the 'hostname' 
or 'ip' and the port that can be parsed by our Address. Once we do that, then 
we shoudn't need any special JSON conversion or parsing.


src/common/http.hpp


Please end sentences with a period.



src/common/http.cpp


Please end sentences with a period. Please do a sweep of your review, I see 
this a handful of places. Looks like a great candidate for a clang-tidy check. 
;-)



src/common/http.cpp


Please use identical names in the declaration and definition: 
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html?showone=Function_Declarations_and_Definitions#Function_Declarations_and_Definitions

There will be a clang-tidy check that we can turn on for this soon, would 
love help with that: 
https://github.com/llvm-mirror/clang-tools-extra/blob/master/clang-tidy/readability/NamedParameterCheck.h



src/common/http.cpp


Please see formatting for braces: 
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Braced_Initializer_List_Format

tl;dr; Use braced-initializer list construction like a constructor.

JSON::Protobuf protobuf{masterInfo};

Please fix throughout this review.

But this brings me to a second question, why use braced-initializer list 
here at all when you're just calling a normal constructor (i.e., not a 
constructor that takes an std::initializer_list or an implicit constructor that 
just sets the fields of the object).

Finally, this can be simplified:

JSON::Object json(JSON::Protobuf(masterInfo));



src/common/http.cpp


This seems rather aggressive, while I understand that 'ip' is required, 
that doesn't mean that someone might not pass an incomplete protobuf where this 
could crash hard instead of just returning an Error.



src/common/http.cpp


Why the extra temporary? Doing the compilers job Marco? ;-)

net::IP ip(ntohl(ipAsInt.get().value));



src/common/parse.hpp


Is this a rouge 'template<>' that shouldn't be there?



src/common/parse.hpp


Operators, including '+' on previous line please.



src/tests/common/parse_tests.cpp


ASSERT_ERROR



src/tests/common/parse_tests.cpp


ASSERT_ERROR


- Benjamin Hindman


On May 31, 2015, 2:58 a.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated May 31, 2015, 2:58 a.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2340
> https://issues.apache.org/jira/browse/MESOS-2340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e7281ac6667562a27b0427b0ba7f41de98702928 
>   src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 
>   src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
>   src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
>   src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
>   src/tests/common/parse_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[haosdent huang]

> On June 1, 2015, 7:08 a.m., haosdent huang wrote:
> > src/Makefile.am, line 1465
> > 
> >
> > seems the order here is follow dictionary order, so how about move them 
> > after `tests/authorization_tests.cpp`?
> 
> Niklas Nielsen wrote:
> Note that this is a subfolder and we traditionally have folder come after 
> files.

@nnielsen, thank you for your explain.


- haosdent


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


On May 31, 2015, 2:58 a.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated May 31, 2015, 2:58 a.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2340
> https://issues.apache.org/jira/browse/MESOS-2340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e7281ac6667562a27b0427b0ba7f41de98702928 
>   src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 
>   src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
>   src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
>   src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
>   src/tests/common/parse_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Niklas Nielsen]

> On June 1, 2015, 12:08 a.m., haosdent huang wrote:
> > src/Makefile.am, line 1465
> > 
> >
> > seems the order here is follow dictionary order, so how about move them 
> > after `tests/authorization_tests.cpp`?

Note that this is a subfolder and we traditionally have folder come after files.


- Niklas


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


On May 30, 2015, 7:58 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated May 30, 2015, 7:58 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2340
> https://issues.apache.org/jira/browse/MESOS-2340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e7281ac6667562a27b0427b0ba7f41de98702928 
>   src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 
>   src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
>   src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
>   src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
>   src/tests/common/parse_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[haosdent huang]

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



src/common/parse.hpp


@adam-mesos said 

> we prefer trailing "+"s rather than leading "+"s.

in this review.


- haosdent huang


On May 31, 2015, 2:58 a.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated May 31, 2015, 2:58 a.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2340
> https://issues.apache.org/jira/browse/MESOS-2340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e7281ac6667562a27b0427b0ba7f41de98702928 
>   src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 
>   src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
>   src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
>   src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
>   src/tests/common/parse_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[haosdent huang]

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

Ship it!


Ship It!

- haosdent huang


On May 31, 2015, 2:58 a.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated May 31, 2015, 2:58 a.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2340
> https://issues.apache.org/jira/browse/MESOS-2340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e7281ac6667562a27b0427b0ba7f41de98702928 
>   src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 
>   src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
>   src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
>   src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
>   src/tests/common/parse_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[haosdent huang]

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



src/common/parse.hpp


How about change 'ID, port and IP' to 'id, port and ip'?


- haosdent huang


On May 31, 2015, 2:58 a.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated May 31, 2015, 2:58 a.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2340
> https://issues.apache.org/jira/browse/MESOS-2340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e7281ac6667562a27b0427b0ba7f41de98702928 
>   src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 
>   src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
>   src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
>   src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
>   src/tests/common/parse_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[haosdent huang]

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



src/Makefile.am


seems the order here is follow dictionary order, so how about move them 
after `tests/authorization_tests.cpp`?


- haosdent huang


On May 31, 2015, 2:58 a.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated May 31, 2015, 2:58 a.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2340
> https://issues.apache.org/jira/browse/MESOS-2340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e7281ac6667562a27b0427b0ba7f41de98702928 
>   src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 
>   src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
>   src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
>   src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
>   src/tests/common/parse_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [34687]

All tests passed.

- Mesos ReviewBot


On May 31, 2015, 2:58 a.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated May 31, 2015, 2:58 a.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2340
> https://issues.apache.org/jira/browse/MESOS-2340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e7281ac6667562a27b0427b0ba7f41de98702928 
>   src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 
>   src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
>   src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
>   src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
>   src/tests/common/parse_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Marco Massenzio]

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

(Updated May 31, 2015, 2:58 a.m.)


Review request for mesos, haosdent huang and Niklas Nielsen.


Changes
---

Fixed an issue with tests


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


Repository: mesos


Description
---

Jira: MESOS-2340

This is a preliminary step to enabling JSON API
for Master Discovery via Zookeeper.

We currently save the MasterInfo PB to ZK
serializing directly the binary data, so that
for clients to retrieve that information, they
need to either link up with libmesos or
obtain the PB definition (in mesos/mesos.proto).

This change only provides the (de)serialization
utility methods and associated tests.


Diffs (updated)
-

  src/Makefile.am e7281ac6667562a27b0427b0ba7f41de98702928 
  src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 
  src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
  src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
  src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
  src/tests/common/parse_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Marco Massenzio

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Marco Massenzio]

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

(Updated May 29, 2015, 9:36 p.m.)


Review request for mesos, haosdent huang and Niklas Nielsen.


Changes
---

Refactored methods into parse.hpp and http.hpp
Added a couple more tests


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


Repository: mesos


Description
---

Jira: MESOS-2340

This is a preliminary step to enabling JSON API
for Master Discovery via Zookeeper.

We currently save the MasterInfo PB to ZK
serializing directly the binary data, so that
for clients to retrieve that information, they
need to either link up with libmesos or
obtain the PB definition (in mesos/mesos.proto).

This change only provides the (de)serialization
utility methods and associated tests.


Diffs (updated)
-

  src/Makefile.am e7281ac6667562a27b0427b0ba7f41de98702928 
  src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 
  src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
  src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
  src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
  src/tests/common/parse_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Marco Massenzio

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Marco Massenzio]

> On May 27, 2015, 2:18 a.m., Niklas Nielsen wrote:
> > src/common/protobuf_utils.hpp, line 83
> > 
> >
> > Shouldn't parse() go in 
> > https://github.com/apache/mesos/blob/master/src/common/parse.hpp?
> 
> Marco Massenzio wrote:
> I don't really know - in the file there's nothing to say one way or the 
> other: it just seems a bunch of `parse()` methods (in the `flagss::` 
> namespace) converting from `string` to `protobuf`?
> I guess so, if one takes the stance the similarly named methods should 
> all be in the same header file...
> 
> I actually prefer to keep methods *logically* grouped and, ideally, the 
> two to/from conversions next to each other, so it's easier to keep them in 
> sync if either formats (JSON / PBuf) ought to change.
> 
> Also, not particularly fond of methods kept inline and in header files - 
> but that's a matter of taste, I can easily be convinced either way.
> 
> Niklas Nielsen wrote:
> We have been catious on landing too many helpers in protobuf_utils in the 
> past.
> If you want to do a logical separation, should it go in a 
> protobuf_parse.hpp instead (taken that it is a pattern we will see more 
> often)?
> I still like the idea to land it in parse.hpp, as it may be Foo -> Bar 
> convertions (is 'parse' the right term, taken that we already have a JSON 
> object? Or is 'convert' more precise?)

well, probably *too* cautious :)
that file has only other *three* methods, and is, overall, including my 
changes, approx 200 lines

If you really feel strongly that we should create yet another file, sure, why 
not - but it seems to me it does not add any value (and it's not like we have 
20 helpers there: there are now like, 5-6, including the two I just created).

I'm not trying to be difficult here, Nik - but really, to create a file to add 
a single method, is it really going to make things better?


- Marco


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


On May 27, 2015, 6:07 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated May 27, 2015, 6:07 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2340
> https://issues.apache.org/jira/browse/MESOS-2340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 814468e3c5c750a6649b5eeb7c7f945f9e025c19 
>   src/common/protobuf_utils.hpp 9ecd2343689252af1b997392ec367d14d55ac7d1 
>   src/common/protobuf_utils.cpp bd6996159e73bf63bb7c2fa3a28def6a2be92b1b 
>   src/tests/common/protobuf_utils_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Niklas Nielsen]

> On May 26, 2015, 7:18 p.m., Niklas Nielsen wrote:
> > src/common/protobuf_utils.hpp, line 83
> > 
> >
> > Shouldn't parse() go in 
> > https://github.com/apache/mesos/blob/master/src/common/parse.hpp?
> 
> Marco Massenzio wrote:
> I don't really know - in the file there's nothing to say one way or the 
> other: it just seems a bunch of `parse()` methods (in the `flagss::` 
> namespace) converting from `string` to `protobuf`?
> I guess so, if one takes the stance the similarly named methods should 
> all be in the same header file...
> 
> I actually prefer to keep methods *logically* grouped and, ideally, the 
> two to/from conversions next to each other, so it's easier to keep them in 
> sync if either formats (JSON / PBuf) ought to change.
> 
> Also, not particularly fond of methods kept inline and in header files - 
> but that's a matter of taste, I can easily be convinced either way.

We have been catious on landing too many helpers in protobuf_utils in the past.
If you want to do a logical separation, should it go in a protobuf_parse.hpp 
instead (taken that it is a pattern we will see more often)?
I still like the idea to land it in parse.hpp, as it may be Foo -> Bar 
convertions (is 'parse' the right term, taken that we already have a JSON 
object? Or is 'convert' more precise?)


- Niklas


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


On May 27, 2015, 11:07 a.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated May 27, 2015, 11:07 a.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2340
> https://issues.apache.org/jira/browse/MESOS-2340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 814468e3c5c750a6649b5eeb7c7f945f9e025c19 
>   src/common/protobuf_utils.hpp 9ecd2343689252af1b997392ec367d14d55ac7d1 
>   src/common/protobuf_utils.cpp bd6996159e73bf63bb7c2fa3a28def6a2be92b1b 
>   src/tests/common/protobuf_utils_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [34687]

All tests passed.

- Mesos ReviewBot


On May 27, 2015, 6:07 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated May 27, 2015, 6:07 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2340
> https://issues.apache.org/jira/browse/MESOS-2340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 814468e3c5c750a6649b5eeb7c7f945f9e025c19 
>   src/common/protobuf_utils.hpp 9ecd2343689252af1b997392ec367d14d55ac7d1 
>   src/common/protobuf_utils.cpp bd6996159e73bf63bb7c2fa3a28def6a2be92b1b 
>   src/tests/common/protobuf_utils_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Marco Massenzio]

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

(Updated May 27, 2015, 6:07 p.m.)


Review request for mesos, haosdent huang and Niklas Nielsen.


Changes
---

Addressed comments
Replaced 'manual' parsing with JSON::Protobuf
Updated tests


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


Repository: mesos


Description
---

Jira: MESOS-2340

This is a preliminary step to enabling JSON API
for Master Discovery via Zookeeper.

We currently save the MasterInfo PB to ZK
serializing directly the binary data, so that
for clients to retrieve that information, they
need to either link up with libmesos or
obtain the PB definition (in mesos/mesos.proto).

This change only provides the (de)serialization
utility methods and associated tests.


Diffs (updated)
-

  src/Makefile.am 814468e3c5c750a6649b5eeb7c7f945f9e025c19 
  src/common/protobuf_utils.hpp 9ecd2343689252af1b997392ec367d14d55ac7d1 
  src/common/protobuf_utils.cpp bd6996159e73bf63bb7c2fa3a28def6a2be92b1b 
  src/tests/common/protobuf_utils_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Marco Massenzio

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Marco Massenzio]

> On May 27, 2015, 2:18 a.m., Niklas Nielsen wrote:
> > src/common/protobuf_utils.cpp, line 169
> > 
> >
> > We have traditionally used stringify(...) if we needed type T to string 
> > conversions. Have you checked if we have one for net::IP?
> 
> Marco Massenzio wrote:
> haven't come across it, and, trust me, I've looked for better ways of 
> getting a string out of a net::IP object
> would it be in the net::IP class, or tucked somewhere else safe?

sweet - found it!


- Marco


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


On May 26, 2015, 11:54 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated May 26, 2015, 11:54 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2340
> https://issues.apache.org/jira/browse/MESOS-2340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 814468e3c5c750a6649b5eeb7c7f945f9e025c19 
>   src/common/protobuf_utils.hpp 9ecd2343689252af1b997392ec367d14d55ac7d1 
>   src/common/protobuf_utils.cpp bd6996159e73bf63bb7c2fa3a28def6a2be92b1b 
>   src/tests/common/protobuf_utils_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Marco Massenzio]

> On May 27, 2015, 2:18 a.m., Niklas Nielsen wrote:
> > src/common/protobuf_utils.hpp, line 83
> > 
> >
> > Shouldn't parse() go in 
> > https://github.com/apache/mesos/blob/master/src/common/parse.hpp?

I don't really know - in the file there's nothing to say one way or the other: 
it just seems a bunch of `parse()` methods (in the `flagss::` namespace) 
converting from `string` to `protobuf`?
I guess so, if one takes the stance the similarly named methods should all be 
in the same header file...

I actually prefer to keep methods *logically* grouped and, ideally, the two 
to/from conversions next to each other, so it's easier to keep them in sync if 
either formats (JSON / PBuf) ought to change.

Also, not particularly fond of methods kept inline and in header files - but 
that's a matter of taste, I can easily be convinced either way.


> On May 27, 2015, 2:18 a.m., Niklas Nielsen wrote:
> > src/common/protobuf_utils.hpp, line 87
> > 
> >
> > We traditionally spell out variable names. Here, minfo would be 
> > masterInfo or simply 'info'

fixed everywhere to `masterInfo` - awfully sorry about that.


> On May 27, 2015, 2:18 a.m., Niklas Nielsen wrote:
> > src/tests/common/protobuf_utils_tests.cpp, line 23
> > 
> >
> > This (and standard c includes) goes first
> 
> Marco Massenzio wrote:
> haven't we changed this recently?
> (I was assuming that the protobuf_utils.hpp was the 'relevant' header?)

ok - fixed this (incidentally, I'd copied the order from `http_tests.cpp` which 
includes `gtest/gtest.h` before ``)
I still assume that `common/protobuf_utils.hpp` can be regarded as the 
`"related header"` and should be first: correct?


- Marco


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


On May 26, 2015, 11:54 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated May 26, 2015, 11:54 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2340
> https://issues.apache.org/jira/browse/MESOS-2340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 814468e3c5c750a6649b5eeb7c7f945f9e025c19 
>   src/common/protobuf_utils.hpp 9ecd2343689252af1b997392ec367d14d55ac7d1 
>   src/common/protobuf_utils.cpp bd6996159e73bf63bb7c2fa3a28def6a2be92b1b 
>   src/tests/common/protobuf_utils_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [34687]

All tests passed.

- Mesos ReviewBot


On May 26, 2015, 11:54 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated May 26, 2015, 11:54 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2340
> https://issues.apache.org/jira/browse/MESOS-2340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 814468e3c5c750a6649b5eeb7c7f945f9e025c19 
>   src/common/protobuf_utils.hpp 9ecd2343689252af1b997392ec367d14d55ac7d1 
>   src/common/protobuf_utils.cpp bd6996159e73bf63bb7c2fa3a28def6a2be92b1b 
>   src/tests/common/protobuf_utils_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Marco Massenzio]

> On May 27, 2015, 2:18 a.m., Niklas Nielsen wrote:
> > src/Makefile.am, line 1387
> > 
> >
> > Is the alignment right here? Try to set your tabstop to 8 :)
> > Also, shouldn't the ordering bring this further down?

no, it's utterly wrong - I planned to fix it later using vim (Eclipse is all 
off with tabs) but forgot.


> On May 27, 2015, 2:18 a.m., Niklas Nielsen wrote:
> > src/common/protobuf_utils.cpp, line 169
> > 
> >
> > We have traditionally used stringify(...) if we needed type T to string 
> > conversions. Have you checked if we have one for net::IP?

haven't come across it, and, trust me, I've looked for better ways of getting a 
string out of a net::IP object
would it be in the net::IP class, or tucked somewhere else safe?


> On May 27, 2015, 2:18 a.m., Niklas Nielsen wrote:
> > src/common/protobuf_utils.cpp, lines 202-203
> > 
> >
> > Could we do this error message inline in the return Error(...)? We can 
> > use stringify() to get the JSON string:
> > 
> > return Error("Missing ..." + stringify(json));

cool!
will do, thanks.


> On May 27, 2015, 2:18 a.m., Niklas Nielsen wrote:
> > src/tests/common/protobuf_utils_tests.cpp, line 23
> > 
> >
> > This (and standard c includes) goes first

haven't we changed this recently?
(I was assuming that the protobuf_utils.hpp was the 'relevant' header?)


> On May 27, 2015, 2:18 a.m., Niklas Nielsen wrote:
> > src/tests/common/protobuf_utils_tests.cpp, line 61
> > 
> >
> > Should we encode this in human readable form?

we probably could, but (a) the code would be pretty gnarly and (b) what's the 
point, really?
this is what that IP string gets turned into when it becomes an int32 - we want 
to make sure this invariant does not get broken by future changes/refactorings.


> On May 27, 2015, 2:18 a.m., Niklas Nielsen wrote:
> > src/tests/common/protobuf_utils_tests.cpp, line 148
> > 
> >
> > What does 'FullCycle' mean?
> > Mind added test descriptions as preceeding comments to the test blocks?

eh eh I honestly spent five minutes trying to come up with something 
meaningful: epic fail, clearly!
it actually means we go from UPID to MasterInfo to JSON and back to MasterInfo, 
and we want to get the same thing we started from.

I thought the comment made that a bit clearer, but will elaborate on the point.


- Marco


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


On May 26, 2015, 11:54 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated May 26, 2015, 11:54 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2340
> https://issues.apache.org/jira/browse/MESOS-2340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 814468e3c5c750a6649b5eeb7c7f945f9e025c19 
>   src/common/protobuf_utils.hpp 9ecd2343689252af1b997392ec367d14d55ac7d1 
>   src/common/protobuf_utils.cpp bd6996159e73bf63bb7c2fa3a28def6a2be92b1b 
>   src/tests/common/protobuf_utils_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Marco Massenzio]

> On May 27, 2015, 2:18 a.m., Niklas Nielsen wrote:
> > Hey Marco,
> > 
> > First pass: One high-level question to your patch: Could we reuse 
> > JSON::Protobuf() to some of the modeling? 
> > https://github.com/apache/mesos/blob/master/3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp#L568
> > 
> > If the IP is wrong type, we can perhaps correct it after parsing it with 
> > JSON::Protobuf()?

Hey Nik - thanks for super-quick review.

Very happy to use JSON::Protobuf - what does it do?
Do you know where I can find some documentation or example usage?

Thanks for pointing me to it - "random walk" code learning is kinda hit 'n miss 
:)
(but the only option when there's no docs around...)


- Marco


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


On May 26, 2015, 11:54 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated May 26, 2015, 11:54 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2340
> https://issues.apache.org/jira/browse/MESOS-2340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 814468e3c5c750a6649b5eeb7c7f945f9e025c19 
>   src/common/protobuf_utils.hpp 9ecd2343689252af1b997392ec367d14d55ac7d1 
>   src/common/protobuf_utils.cpp bd6996159e73bf63bb7c2fa3a28def6a2be92b1b 
>   src/tests/common/protobuf_utils_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Niklas Nielsen]

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


Hey Marco,

First pass: One high-level question to your patch: Could we reuse 
JSON::Protobuf() to some of the modeling? 
https://github.com/apache/mesos/blob/master/3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp#L568

If the IP is wrong type, we can perhaps correct it after parsing it with 
JSON::Protobuf()?


src/Makefile.am


Is the alignment right here? Try to set your tabstop to 8 :)
Also, shouldn't the ordering bring this further down?



src/common/protobuf_utils.hpp


Shouldn't parse() go in 
https://github.com/apache/mesos/blob/master/src/common/parse.hpp?



src/common/protobuf_utils.hpp


We traditionally spell out variable names. Here, minfo would be masterInfo 
or simply 'info'



src/common/protobuf_utils.cpp


We have traditionally used stringify(...) if we needed type T to string 
conversions. Have you checked if we have one for net::IP?



src/common/protobuf_utils.cpp


Two newlines



src/common/protobuf_utils.cpp


Could we do this error message inline in the return Error(...)? We can use 
stringify() to get the JSON string:

return Error("Missing ..." + stringify(json));



src/tests/common/protobuf_utils_tests.cpp


This (and standard c includes) goes first



src/tests/common/protobuf_utils_tests.cpp


We usually spell out variable names. MasterInfo info or MasterInfo 
masterInfo, would be the to-go names here :)



src/tests/common/protobuf_utils_tests.cpp


Should we encode this in human readable form?



src/tests/common/protobuf_utils_tests.cpp


This can be made const, right?



src/tests/common/protobuf_utils_tests.cpp


Let's move this closer to where we use it (line 141)



src/tests/common/protobuf_utils_tests.cpp


What does 'FullCycle' mean?
Mind added test descriptions as preceeding comments to the test blocks?


- Niklas Nielsen


On May 26, 2015, 4:54 p.m., Marco Massenzio wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> ---
> 
> (Updated May 26, 2015, 4:54 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2340
> https://issues.apache.org/jira/browse/MESOS-2340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 814468e3c5c750a6649b5eeb7c7f945f9e025c19 
>   src/common/protobuf_utils.hpp 9ecd2343689252af1b997392ec367d14d55ac7d1 
>   src/common/protobuf_utils.cpp bd6996159e73bf63bb7c2fa3a28def6a2be92b1b 
>   src/tests/common/protobuf_utils_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Review Request 34687: (De)Serializing MasterInfo PB to JSON

[Marco Massenzio]

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

Review request for mesos, haosdent huang and Niklas Nielsen.


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


Repository: mesos


Description
---

Jira: MESOS-2340

This is a preliminary step to enabling JSON API
for Master Discovery via Zookeeper.

We currently save the MasterInfo PB to ZK
serializing directly the binary data, so that
for clients to retrieve that information, they
need to either link up with libmesos or
obtain the PB definition (in mesos/mesos.proto).

This change only provides the (de)serialization
utility methods and associated tests.


Diffs
-

  src/Makefile.am 814468e3c5c750a6649b5eeb7c7f945f9e025c19 
  src/common/protobuf_utils.hpp 9ecd2343689252af1b997392ec367d14d55ac7d1 
  src/common/protobuf_utils.cpp bd6996159e73bf63bb7c2fa3a28def6a2be92b1b 
  src/tests/common/protobuf_utils_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Marco Massenzio