subject:"Re\\\: Review Request 38342\\\: Add JSON\\\:\\\:protobuf for google\\\:\\\:protobuf\\\:\\\:RepeatedPtrField \\\(stout part\\\)"

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-11-04 Thread Klaus Ma


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

(Updated Nov. 4, 2015, 9:19 p.m.)


Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.


Changes
---

rebase the code


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


Repository: mesos


Description
---

Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
converts a `google::protobuf::Message` into a `JSON::Object`.
We should add the support for `google::protobuf::RepeatedPtrField` by 
introducing overloaded functions.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp f0e7870 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 

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


Testing
---

cd 3rdparty/libprocess/3rdparty/stout
./boostrap
./configure
make


Thanks,

Klaus Ma

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-11-02 Thread Michael Park


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

Ship it!


Ship It!

- Michael Park


On Oct. 20, 2015, 2:39 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Oct. 20, 2015, 2:39 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp f0e7870 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 
> 
> Diff: https://reviews.apache.org/r/38342/diff/
> 
> 
> Testing
> ---
> 
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-10-19 Thread Michael Park



> On Oct. 18, 2015, 11:02 p.m., Michael Park wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp, lines 182-189
> > 
> >
> > Indent 2 spaces, not the strings themselves, but within the strings.
> 
> Klaus Ma wrote:
> The 2 sparces indent are in the strings, do you mean we should move it 
> out?

I mean at the `[ ... ]` level:

```cpp
"["
"  {"
"\"id\": \"message1\","
"\"numbers\": [1, 2]"
"  },"
"  {"
"\"id\": \"message2\","
"\"numbers\": [1, 2]"
"  }"
"]"
```


- Michael


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


On Oct. 16, 2015, 9:56 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Oct. 16, 2015, 9:56 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 
> 
> Diff: https://reviews.apache.org/r/38342/diff/
> 
> 
> Testing
> ---
> 
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-10-19 Thread Michael Park



> On Oct. 18, 2015, 11:02 p.m., Michael Park wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, lines 764-768
> > 
> >
> > As mentioned before, let's remove the `default` case entirely. Refer to 
> > [MESOS-3754](https://issues.apache.org/jira/browse/MESOS-3754) for details.
> 
> Klaus Ma wrote:
> I just remove `default:` becase keep `ABORT` for deprecated case 
> (`google::protobuf::FieldDescriptor::TYPE_GROUP:`).

Yep, that's good. Thanks.


- Michael


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


On Oct. 16, 2015, 9:56 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Oct. 16, 2015, 9:56 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 
> 
> Diff: https://reviews.apache.org/r/38342/diff/
> 
> 
> Testing
> ---
> 
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-10-19 Thread Klaus Ma


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


The code diff of #38335 are alos upedted based on master.

- Klaus Ma


On Oct. 20, 2015, 2:39 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Oct. 20, 2015, 2:39 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp f0e7870 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 
> 
> Diff: https://reviews.apache.org/r/38342/diff/
> 
> 
> Testing
> ---
> 
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-10-19 Thread Klaus Ma


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

(Updated Oct. 20, 2015, 2:39 a.m.)


Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.


Changes
---

Address comments


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


Repository: mesos


Description
---

Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
converts a `google::protobuf::Message` into a `JSON::Object`.
We should add the support for `google::protobuf::RepeatedPtrField` by 
introducing overloaded functions.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp f0e7870 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 

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


Testing
---

cd 3rdparty/libprocess/3rdparty/stout
./boostrap
./configure
make


Thanks,

Klaus Ma

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-10-18 Thread Klaus Ma



> On Oct. 18, 2015, 11:02 p.m., Michael Park wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, lines 764-768
> > 
> >
> > As mentioned before, let's remove the `default` case entirely. Refer to 
> > [MESOS-3754](https://issues.apache.org/jira/browse/MESOS-3754) for details.

I just remove `default:` becase keep `ABORT` for deprecated case 
(`google::protobuf::FieldDescriptor::TYPE_GROUP:`).


> On Oct. 18, 2015, 11:02 p.m., Michael Park wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp, lines 182-189
> > 
> >
> > Indent 2 spaces, not the strings themselves, but within the strings.

The 2 sparces indent are in the strings, do you mean we should move it out?


- Klaus


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


On Oct. 16, 2015, 9:56 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Oct. 16, 2015, 9:56 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 
> 
> Diff: https://reviews.apache.org/r/38342/diff/
> 
> 
> Testing
> ---
> 
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-10-18 Thread Michael Park


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



3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (lines 762 - 766)


As mentioned before, let's remove the `default` case entirely. Refer to 
[MESOS-3754](https://issues.apache.org/jira/browse/MESOS-3754) for details.



3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp (lines 182 - 189)


Indent 2 spaces, not the strings themselves, but within the strings.


- Michael Park


On Oct. 16, 2015, 9:56 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Oct. 16, 2015, 9:56 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 
> 
> Diff: https://reviews.apache.org/r/38342/diff/
> 
> 
> Testing
> ---
> 
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-10-16 Thread Klaus Ma


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

(Updated Oct. 16, 2015, 9:56 a.m.)


Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
converts a `google::protobuf::Message` into a `JSON::Object`.
We should add the support for `google::protobuf::RepeatedPtrField` by 
introducing overloaded functions.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 

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


Testing
---

cd 3rdparty/libprocess/3rdparty/stout
./boostrap
./configure
make


Thanks,

Klaus Ma

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-10-12 Thread Alexander Rukletsov



> On Sept. 29, 2015, 10:43 a.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, lines 773-774
> > 
> >
> > Could you please help me understand why this check is necessary? IIUC, 
> > compiler ensures `T` is a protobuf becasue you pass `elem` of type `T` to 
> > `JSON::protobuf()`.
> > 
> > AFAIK, the only reason to do this check is to prohibit arguments like 
> > `google::protobuf::RepeatedPtrField`.
> >  Do you want to express this? I don't think it is necessary, because 
> > `JSON::Array` filled with `JSON::Array` is fine.
> > 
> > @MPark, what'd you say?
> 
> Klaus Ma wrote:
> To me, this's a safe guard to make sure this function is not abused.
> For 
> `google::protobuf::RepeatedPtrField`, 
> I think we need to define a message to include 
> `google::protobuf::RepeatedPtrField`, the proto file looks like:
> 
> message MyMessage
> {
> message Item {
> string value = 1;
> }
> repeated Item values = 1;
> }
> 
> Alexander Rukletsov wrote:
> Hmmm, I see your point. I would leave this decision to your shepherd!
> 
> Michael Park wrote:
> While `RepeatedPtrField` => `Array` is a 
> fine mapping in and of itself, `RepeatedPtrField` would 
> not arise from a legitimate protobuf definition. In order to prevent from 
> manually constructed instances of `RepeatedPtrField` 
> being passed around, I'm for keeping the `static_assert` here.

Let's document this in the code. After that feel free to fix/drop the issue.


- Alexander


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


On Oct. 4, 2015, 11:29 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Oct. 4, 2015, 11:29 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 
> 
> Diff: https://reviews.apache.org/r/38342/diff/
> 
> 
> Testing
> ---
> 
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-10-12 Thread Alexander Rukletsov


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

Ship it!



3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (lines 777 - 778)


s/google::protobuf::Message/protobuf message/ for consistency?



3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (line 778)


Newline before



3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (lines 785 - 786)


Newline, please!


- Alexander Rukletsov


On Oct. 4, 2015, 11:29 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Oct. 4, 2015, 11:29 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 
> 
> Diff: https://reviews.apache.org/r/38342/diff/
> 
> 
> Testing
> ---
> 
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-10-12 Thread Alexander Rukletsov



> On Sept. 29, 2015, 12:14 p.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, lines 762-763
> > 
> >
> > I've seen your discussion with @Jan above, here is what I think 
> > regarding `ABORT` vs. `Try<>`. 
> > 
> > First off, some background. We tend to use `ABORT()` and `CHECK_*` 
> > macros when so-called "internal invariant" is violated, for example an 
> > object is being used without proper initialization, or a change is being 
> > made to an instance, that does not exists any more. On the other side 
> > `Try<>` and `Error<>` family is used when our expectation about the outer 
> > world does not hold, or, in other words, when an action cannot be 
> > completed, but no internal invariant is broken. User input, I/O, network 
> > operations are good examples of the latter case.
> > 
> > Let's try to figure out what we have here. I would say, it's an 
> > internal invariant, and here is why. JSON is less strict as Protobuf, 
> > therefore conversion Protobuf->JSON always exists (note that this is not 
> > symmetrical, JSON->Protobuf is not always possible, hence we use `Try<>` in 
> > e.g. `protobuf::parse<>()`). The only reason it may fail (remember we do 
> > not handle OOM exceptions) is because we convert a protobuf message of a 
> > yet unknown format (most probably future proto versions), which is a 
> > violation of an internal invariant!
> > 
> > Therefore I would suggest we keep `ABORT()` but document in the 
> > preambular comment why we use `ABORT()` and not `Try<>`, explaining what 
> > internal invariant is in this case.

I would like us to document the motivation for `ABORT()` here.


- Alexander


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


On Oct. 4, 2015, 11:29 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Oct. 4, 2015, 11:29 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 
> 
> Diff: https://reviews.apache.org/r/38342/diff/
> 
> 
> Testing
> ---
> 
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-10-06 Thread Klaus Ma



> On Sept. 29, 2015, 4:17 p.m., Alexander Rukletsov wrote:
> > One high level suggestion.
> > 
> > After looking at our http code, I realized that we use the same pattern 
> > again and again, for example:
> > ```
> > JSON::Array array;
> > array.values.reserve(status.network_infos().size()); // MESOS-2353.
> > foreach (const NetworkInfo& info, status.network_infos()) {
> >   array.values.push_back(model(info));
> > }
> > object.values["network_infos"] = std::move(array);
> > ```
> > We cannot use newly added `JSON::protobuf()` here, because a different way 
> > for rendering JSON from protobuf is used. Without digging deep inside, I 
> > know three ways how we create a `JSON` out of a proto in our codebase:
> > - wrap in `JSON::Protobuf()` for individual messages;
> > - wrap in one of the `model()` family functions;
> > - pass as it is for built-in types.
> > 
> > The proposed conversion function covers one of the possible ways. How about 
> > add one more convertion? Something like:
> > ```
> > template 
> > Array protobuf(const google::protobuf::RepeatedPtrField& repeated,
> > const lambda::function(const T&)>& converter)
> > {
> >   static_assert(std::is_convertible::value,
> > "T must be a google::protobuf::Message");
> >   JSON::Array array;
> >   array.values.reserve(repeated.size());
> >   foreach (const T& elem, repeated) {
> > array.values.push_back(converter(elem));
> >   }
> >   
> >   return array;
> > }
> > ```
> > 
> > Then the snippet above could be rewritten as:
> > ```
> > object.values["network_infos"] = 
> > std::move(JSON::protobuf(status.network_infos(), [](const NetworkInfo& 
> > info) { return model(info); });
> > ```
> > 
> > A further improvement would be to accept any iterable collection, not only 
> > `RepeatedPtrField<>`, for example `hashset`.
> > 
> > What do you think?
> 
> Klaus Ma wrote:
> Awesome! I've also try similar proposal, but failed when `function` 
> converting with `template`; your suggestion using lambda is great!
> For the `hashset`, I'd suggest to address it when we have such case in 
> our code :).
> 
> I'll also address your comments above.
> 
> Alexander Rukletsov wrote:
> We do have such cases in our codebase ; ). Here are a few as an example:
> - https://github.com/apache/mesos/blob/master/src/master/http.cpp#L217
> - https://github.com/apache/mesos/blob/master/src/master/http.cpp#L229
> 
> Michael Park wrote:
> I like the idea of taking a projection function as an argument, but let's 
> do it as a separate ticket to keep the scope of this review narrow.
> 
> Klaus Ma wrote:
> I'll log a new ticket to trace projection fuction part.
> 
> Klaus Ma wrote:
> MESOS-3580 is logged to trace this requirement, @alex-mesos, would you 
> shepherd it?
> 
> Alexander Rukletsov wrote:
> Great, thanks! I'm not a Mesos committer, hence I cannot shepherd, but I 
> would love to review and help out with that! Maybe @MPark will agree to 
> shepherd?
> 
> Michael Park wrote:
> @klaus1982: Thanks for filing MESOS-3580. I think we'll probably hold off 
> working on that ticket to see if it'll still be useful after the work around 
> speeding up JSON parsing/streaming.

OK, got it for MESOS-3580.


- Klaus


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


On Oct. 4, 2015, 11:29 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Oct. 4, 2015, 11:29 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 
> 
> Diff: https://reviews.apache.org/r/38342/diff/
> 
> 
> Testing
> ---
> 
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-10-05 Thread Michael Park



> On Sept. 28, 2015, 9:11 a.m., Jan Schlicht wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, line 762
> > 
> >
> > Because this code has been changed from a constructor to a function, we 
> > can do better than calling `ABORT` here. We can change the function 
> > signature to return a `Try` and call `Error` here instead of 
> > `ABORT`. For example see the `write` function in line 55.
> > 
> > The function signature would change to
> > `inline Try protobuf(const google::protobuf::Message& message)`
> 
> Klaus Ma wrote:
> Got your point :).
> Just one concern on how to use it? We did not handle its result if failed 
> in `src`; personally, this's a kind of code practice to avoid undefined 
> protobuf field type. Anyway, I'll update accordingly if necessary.
> 
> Jan Schlicht wrote:
> Well, unfortunately this change means that the code that is using this 
> function has to be changed to handle the error cases. This means checking if 
> the `Try<>` contains a value or an error and react accordingly.
> 
> Klaus Ma wrote:
> Did you check the code diff of #38335? In #38335, any suggestion on the 
> function that return void; those function did not expect json parsing will 
> fail. Personally, it's a bit overkill to re-implement them for json paring 
> error handling which are not expect failed. 
> 
> @alex-mesos, @mcypark, what's your suggestion?

My suggestion here is to leave out the `default:` case since all alternatives 
are being handled. This was discussed in 
[MESOS-26664](https://issues.apache.org/jira/browse/MESOS-2664), and here is an 
[example](https://github.com/apache/mesos/blob/7f352ef886f3116e4bef23b235d87b3182354908/src/common/http.cpp#L53).


- Michael


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


On Oct. 4, 2015, 11:29 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Oct. 4, 2015, 11:29 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 
> 
> Diff: https://reviews.apache.org/r/38342/diff/
> 
> 
> Testing
> ---
> 
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-10-05 Thread Michael Park



> On Sept. 29, 2015, 4:17 p.m., Alexander Rukletsov wrote:
> > One high level suggestion.
> > 
> > After looking at our http code, I realized that we use the same pattern 
> > again and again, for example:
> > ```
> > JSON::Array array;
> > array.values.reserve(status.network_infos().size()); // MESOS-2353.
> > foreach (const NetworkInfo& info, status.network_infos()) {
> >   array.values.push_back(model(info));
> > }
> > object.values["network_infos"] = std::move(array);
> > ```
> > We cannot use newly added `JSON::protobuf()` here, because a different way 
> > for rendering JSON from protobuf is used. Without digging deep inside, I 
> > know three ways how we create a `JSON` out of a proto in our codebase:
> > - wrap in `JSON::Protobuf()` for individual messages;
> > - wrap in one of the `model()` family functions;
> > - pass as it is for built-in types.
> > 
> > The proposed conversion function covers one of the possible ways. How about 
> > add one more convertion? Something like:
> > ```
> > template 
> > Array protobuf(const google::protobuf::RepeatedPtrField& repeated,
> > const lambda::function(const T&)>& converter)
> > {
> >   static_assert(std::is_convertible::value,
> > "T must be a google::protobuf::Message");
> >   JSON::Array array;
> >   array.values.reserve(repeated.size());
> >   foreach (const T& elem, repeated) {
> > array.values.push_back(converter(elem));
> >   }
> >   
> >   return array;
> > }
> > ```
> > 
> > Then the snippet above could be rewritten as:
> > ```
> > object.values["network_infos"] = 
> > std::move(JSON::protobuf(status.network_infos(), [](const NetworkInfo& 
> > info) { return model(info); });
> > ```
> > 
> > A further improvement would be to accept any iterable collection, not only 
> > `RepeatedPtrField<>`, for example `hashset`.
> > 
> > What do you think?
> 
> Klaus Ma wrote:
> Awesome! I've also try similar proposal, but failed when `function` 
> converting with `template`; your suggestion using lambda is great!
> For the `hashset`, I'd suggest to address it when we have such case in 
> our code :).
> 
> I'll also address your comments above.
> 
> Alexander Rukletsov wrote:
> We do have such cases in our codebase ; ). Here are a few as an example:
> - https://github.com/apache/mesos/blob/master/src/master/http.cpp#L217
> - https://github.com/apache/mesos/blob/master/src/master/http.cpp#L229
> 
> Michael Park wrote:
> I like the idea of taking a projection function as an argument, but let's 
> do it as a separate ticket to keep the scope of this review narrow.
> 
> Klaus Ma wrote:
> I'll log a new ticket to trace projection fuction part.
> 
> Klaus Ma wrote:
> MESOS-3580 is logged to trace this requirement, @alex-mesos, would you 
> shepherd it?
> 
> Alexander Rukletsov wrote:
> Great, thanks! I'm not a Mesos committer, hence I cannot shepherd, but I 
> would love to review and help out with that! Maybe @MPark will agree to 
> shepherd?

@klaus1982: Thanks for filing MESOS-3580. I think we'll probably hold off 
working on that ticket to see if it'll still be useful after the work around 
speeding up JSON parsing/streaming.


- Michael


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


On Oct. 4, 2015, 11:29 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Oct. 4, 2015, 11:29 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 
> 
> Diff: https://reviews.apache.org/r/38342/diff/
> 
> 
> Testing
> ---
> 
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-10-05 Thread Alexander Rukletsov



> On Sept. 29, 2015, 4:17 p.m., Alexander Rukletsov wrote:
> > One high level suggestion.
> > 
> > After looking at our http code, I realized that we use the same pattern 
> > again and again, for example:
> > ```
> > JSON::Array array;
> > array.values.reserve(status.network_infos().size()); // MESOS-2353.
> > foreach (const NetworkInfo& info, status.network_infos()) {
> >   array.values.push_back(model(info));
> > }
> > object.values["network_infos"] = std::move(array);
> > ```
> > We cannot use newly added `JSON::protobuf()` here, because a different way 
> > for rendering JSON from protobuf is used. Without digging deep inside, I 
> > know three ways how we create a `JSON` out of a proto in our codebase:
> > - wrap in `JSON::Protobuf()` for individual messages;
> > - wrap in one of the `model()` family functions;
> > - pass as it is for built-in types.
> > 
> > The proposed conversion function covers one of the possible ways. How about 
> > add one more convertion? Something like:
> > ```
> > template 
> > Array protobuf(const google::protobuf::RepeatedPtrField& repeated,
> > const lambda::function(const T&)>& converter)
> > {
> >   static_assert(std::is_convertible::value,
> > "T must be a google::protobuf::Message");
> >   JSON::Array array;
> >   array.values.reserve(repeated.size());
> >   foreach (const T& elem, repeated) {
> > array.values.push_back(converter(elem));
> >   }
> >   
> >   return array;
> > }
> > ```
> > 
> > Then the snippet above could be rewritten as:
> > ```
> > object.values["network_infos"] = 
> > std::move(JSON::protobuf(status.network_infos(), [](const NetworkInfo& 
> > info) { return model(info); });
> > ```
> > 
> > A further improvement would be to accept any iterable collection, not only 
> > `RepeatedPtrField<>`, for example `hashset`.
> > 
> > What do you think?
> 
> Klaus Ma wrote:
> Awesome! I've also try similar proposal, but failed when `function` 
> converting with `template`; your suggestion using lambda is great!
> For the `hashset`, I'd suggest to address it when we have such case in 
> our code :).
> 
> I'll also address your comments above.
> 
> Alexander Rukletsov wrote:
> We do have such cases in our codebase ; ). Here are a few as an example:
> - https://github.com/apache/mesos/blob/master/src/master/http.cpp#L217
> - https://github.com/apache/mesos/blob/master/src/master/http.cpp#L229
> 
> Michael Park wrote:
> I like the idea of taking a projection function as an argument, but let's 
> do it as a separate ticket to keep the scope of this review narrow.
> 
> Klaus Ma wrote:
> I'll log a new ticket to trace projection fuction part.
> 
> Klaus Ma wrote:
> MESOS-3580 is logged to trace this requirement, @alex-mesos, would you 
> shepherd it?

Great, thanks! I'm not a Mesos committer, hence I cannot shepherd, but I would 
love to review and help out with that! Maybe @MPark will agree to shepherd?


- Alexander


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


On Oct. 4, 2015, 11:29 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Oct. 4, 2015, 11:29 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 
> 
> Diff: https://reviews.apache.org/r/38342/diff/
> 
> 
> Testing
> ---
> 
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-10-05 Thread Jan Schlicht


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

Ship it!


Regarding the "`ABORT` or `Try<>`" discussion, I agree with your comments and 
the ones from AlexR. It's fine to use `ABORT` in this use case. But please add 
a small comment, describing why `ABORT` is used. Something like "We abort here 
instead of using a Try as return value, because we expect this code path to 
never be taken.". This would resolve the open issues.
Other than that, it's a "Ship It!"

- Jan Schlicht


On Oct. 4, 2015, 1:29 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Oct. 4, 2015, 1:29 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 
> 
> Diff: https://reviews.apache.org/r/38342/diff/
> 
> 
> Testing
> ---
> 
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-10-05 Thread Alexander Rukletsov



> On Sept. 29, 2015, 10:43 a.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, lines 773-774
> > 
> >
> > Could you please help me understand why this check is necessary? IIUC, 
> > compiler ensures `T` is a protobuf becasue you pass `elem` of type `T` to 
> > `JSON::protobuf()`.
> > 
> > AFAIK, the only reason to do this check is to prohibit arguments like 
> > `google::protobuf::RepeatedPtrField`.
> >  Do you want to express this? I don't think it is necessary, because 
> > `JSON::Array` filled with `JSON::Array` is fine.
> > 
> > @MPark, what'd you say?
> 
> Klaus Ma wrote:
> To me, this's a safe guard to make sure this function is not abused.
> For 
> `google::protobuf::RepeatedPtrField`, 
> I think we need to define a message to include 
> `google::protobuf::RepeatedPtrField`, the proto file looks like:
> 
> message MyMessage
> {
> message Item {
> string value = 1;
> }
> repeated Item values = 1;
> }

Hmmm, I see your point. I would leave this decision to your shepherd!


- Alexander


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


On Oct. 4, 2015, 11:29 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Oct. 4, 2015, 11:29 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 
> 
> Diff: https://reviews.apache.org/r/38342/diff/
> 
> 
> Testing
> ---
> 
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-10-04 Thread Klaus Ma



> On Sept. 29, 2015, 4:17 p.m., Alexander Rukletsov wrote:
> > One high level suggestion.
> > 
> > After looking at our http code, I realized that we use the same pattern 
> > again and again, for example:
> > ```
> > JSON::Array array;
> > array.values.reserve(status.network_infos().size()); // MESOS-2353.
> > foreach (const NetworkInfo& info, status.network_infos()) {
> >   array.values.push_back(model(info));
> > }
> > object.values["network_infos"] = std::move(array);
> > ```
> > We cannot use newly added `JSON::protobuf()` here, because a different way 
> > for rendering JSON from protobuf is used. Without digging deep inside, I 
> > know three ways how we create a `JSON` out of a proto in our codebase:
> > - wrap in `JSON::Protobuf()` for individual messages;
> > - wrap in one of the `model()` family functions;
> > - pass as it is for built-in types.
> > 
> > The proposed conversion function covers one of the possible ways. How about 
> > add one more convertion? Something like:
> > ```
> > template 
> > Array protobuf(const google::protobuf::RepeatedPtrField& repeated,
> > const lambda::function(const T&)>& converter)
> > {
> >   static_assert(std::is_convertible::value,
> > "T must be a google::protobuf::Message");
> >   JSON::Array array;
> >   array.values.reserve(repeated.size());
> >   foreach (const T& elem, repeated) {
> > array.values.push_back(converter(elem));
> >   }
> >   
> >   return array;
> > }
> > ```
> > 
> > Then the snippet above could be rewritten as:
> > ```
> > object.values["network_infos"] = 
> > std::move(JSON::protobuf(status.network_infos(), [](const NetworkInfo& 
> > info) { return model(info); });
> > ```
> > 
> > A further improvement would be to accept any iterable collection, not only 
> > `RepeatedPtrField<>`, for example `hashset`.
> > 
> > What do you think?
> 
> Klaus Ma wrote:
> Awesome! I've also try similar proposal, but failed when `function` 
> converting with `template`; your suggestion using lambda is great!
> For the `hashset`, I'd suggest to address it when we have such case in 
> our code :).
> 
> I'll also address your comments above.
> 
> Alexander Rukletsov wrote:
> We do have such cases in our codebase ; ). Here are a few as an example:
> - https://github.com/apache/mesos/blob/master/src/master/http.cpp#L217
> - https://github.com/apache/mesos/blob/master/src/master/http.cpp#L229
> 
> Michael Park wrote:
> I like the idea of taking a projection function as an argument, but let's 
> do it as a separate ticket to keep the scope of this review narrow.
> 
> Klaus Ma wrote:
> I'll log a new ticket to trace projection fuction part.

MESOS-3580 is logged to trace this requirement, @alex-mesos, would you shepherd 
it?


- Klaus


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


On Oct. 4, 2015, 11:29 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Oct. 4, 2015, 11:29 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 
> 
> Diff: https://reviews.apache.org/r/38342/diff/
> 
> 
> Testing
> ---
> 
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-10-04 Thread Klaus Ma



> On Sept. 29, 2015, 10:43 a.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp, line 166
> > 
> >
> > See my comment above. I think you should not repeat testing conversion 
> > for all the fields, just a simple message with repeated field.

Replaced with SimpleMessage.


- Klaus


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


On Oct. 4, 2015, 11:29 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Oct. 4, 2015, 11:29 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 
> 
> Diff: https://reviews.apache.org/r/38342/diff/
> 
> 
> Testing
> ---
> 
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-10-04 Thread Klaus Ma



> On Sept. 29, 2015, 4:17 p.m., Alexander Rukletsov wrote:
> > One high level suggestion.
> > 
> > After looking at our http code, I realized that we use the same pattern 
> > again and again, for example:
> > ```
> > JSON::Array array;
> > array.values.reserve(status.network_infos().size()); // MESOS-2353.
> > foreach (const NetworkInfo& info, status.network_infos()) {
> >   array.values.push_back(model(info));
> > }
> > object.values["network_infos"] = std::move(array);
> > ```
> > We cannot use newly added `JSON::protobuf()` here, because a different way 
> > for rendering JSON from protobuf is used. Without digging deep inside, I 
> > know three ways how we create a `JSON` out of a proto in our codebase:
> > - wrap in `JSON::Protobuf()` for individual messages;
> > - wrap in one of the `model()` family functions;
> > - pass as it is for built-in types.
> > 
> > The proposed conversion function covers one of the possible ways. How about 
> > add one more convertion? Something like:
> > ```
> > template 
> > Array protobuf(const google::protobuf::RepeatedPtrField& repeated,
> > const lambda::function(const T&)>& converter)
> > {
> >   static_assert(std::is_convertible::value,
> > "T must be a google::protobuf::Message");
> >   JSON::Array array;
> >   array.values.reserve(repeated.size());
> >   foreach (const T& elem, repeated) {
> > array.values.push_back(converter(elem));
> >   }
> >   
> >   return array;
> > }
> > ```
> > 
> > Then the snippet above could be rewritten as:
> > ```
> > object.values["network_infos"] = 
> > std::move(JSON::protobuf(status.network_infos(), [](const NetworkInfo& 
> > info) { return model(info); });
> > ```
> > 
> > A further improvement would be to accept any iterable collection, not only 
> > `RepeatedPtrField<>`, for example `hashset`.
> > 
> > What do you think?
> 
> Klaus Ma wrote:
> Awesome! I've also try similar proposal, but failed when `function` 
> converting with `template`; your suggestion using lambda is great!
> For the `hashset`, I'd suggest to address it when we have such case in 
> our code :).
> 
> I'll also address your comments above.
> 
> Alexander Rukletsov wrote:
> We do have such cases in our codebase ; ). Here are a few as an example:
> - https://github.com/apache/mesos/blob/master/src/master/http.cpp#L217
> - https://github.com/apache/mesos/blob/master/src/master/http.cpp#L229
> 
> Michael Park wrote:
> I like the idea of taking a projection function as an argument, but let's 
> do it as a separate ticket to keep the scope of this review narrow.

I'll log a new ticket to trace projection fuction part.


- Klaus


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


On Oct. 4, 2015, 11:29 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Oct. 4, 2015, 11:29 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 
> 
> Diff: https://reviews.apache.org/r/38342/diff/
> 
> 
> Testing
> ---
> 
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-10-04 Thread Klaus Ma


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

(Updated Oct. 4, 2015, 11:29 a.m.)


Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.


Changes
---

Address the comments


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


Repository: mesos


Description
---

Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
converts a `google::protobuf::Message` into a `JSON::Object`.
We should add the support for `google::protobuf::RepeatedPtrField` by 
introducing overloaded functions.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 

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


Testing
---

cd 3rdparty/libprocess/3rdparty/stout
./boostrap
./configure
make


Thanks,

Klaus Ma

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-09-30 Thread Alexander Rukletsov



> On Sept. 29, 2015, 4:17 p.m., Alexander Rukletsov wrote:
> > One high level suggestion.
> > 
> > After looking at our http code, I realized that we use the same pattern 
> > again and again, for example:
> > ```
> > JSON::Array array;
> > array.values.reserve(status.network_infos().size()); // MESOS-2353.
> > foreach (const NetworkInfo& info, status.network_infos()) {
> >   array.values.push_back(model(info));
> > }
> > object.values["network_infos"] = std::move(array);
> > ```
> > We cannot use newly added `JSON::protobuf()` here, because a different way 
> > for rendering JSON from protobuf is used. Without digging deep inside, I 
> > know three ways how we create a `JSON` out of a proto in our codebase:
> > - wrap in `JSON::Protobuf()` for individual messages;
> > - wrap in one of the `model()` family functions;
> > - pass as it is for built-in types.
> > 
> > The proposed conversion function covers one of the possible ways. How about 
> > add one more convertion? Something like:
> > ```
> > template 
> > Array protobuf(const google::protobuf::RepeatedPtrField& repeated,
> > const lambda::function(const T&)>& converter)
> > {
> >   static_assert(std::is_convertible::value,
> > "T must be a google::protobuf::Message");
> >   JSON::Array array;
> >   array.values.reserve(repeated.size());
> >   foreach (const T& elem, repeated) {
> > array.values.push_back(converter(elem));
> >   }
> >   
> >   return array;
> > }
> > ```
> > 
> > Then the snippet above could be rewritten as:
> > ```
> > object.values["network_infos"] = 
> > std::move(JSON::protobuf(status.network_infos(), [](const NetworkInfo& 
> > info) { return model(info); });
> > ```
> > 
> > A further improvement would be to accept any iterable collection, not only 
> > `RepeatedPtrField<>`, for example `hashset`.
> > 
> > What do you think?
> 
> Klaus Ma wrote:
> Awesome! I've also try similar proposal, but failed when `function` 
> converting with `template`; your suggestion using lambda is great!
> For the `hashset`, I'd suggest to address it when we have such case in 
> our code :).
> 
> I'll also address your comments above.

We do have such cases in our codebase ; ). Here are a few as an example:
- https://github.com/apache/mesos/blob/master/src/master/http.cpp#L217
- https://github.com/apache/mesos/blob/master/src/master/http.cpp#L229


- Alexander


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


On Sept. 27, 2015, 1:34 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Sept. 27, 2015, 1:34 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 
> 
> Diff: https://reviews.apache.org/r/38342/diff/
> 
> 
> Testing
> ---
> 
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-09-30 Thread Michael Park



> On Sept. 29, 2015, 4:17 p.m., Alexander Rukletsov wrote:
> > One high level suggestion.
> > 
> > After looking at our http code, I realized that we use the same pattern 
> > again and again, for example:
> > ```
> > JSON::Array array;
> > array.values.reserve(status.network_infos().size()); // MESOS-2353.
> > foreach (const NetworkInfo& info, status.network_infos()) {
> >   array.values.push_back(model(info));
> > }
> > object.values["network_infos"] = std::move(array);
> > ```
> > We cannot use newly added `JSON::protobuf()` here, because a different way 
> > for rendering JSON from protobuf is used. Without digging deep inside, I 
> > know three ways how we create a `JSON` out of a proto in our codebase:
> > - wrap in `JSON::Protobuf()` for individual messages;
> > - wrap in one of the `model()` family functions;
> > - pass as it is for built-in types.
> > 
> > The proposed conversion function covers one of the possible ways. How about 
> > add one more convertion? Something like:
> > ```
> > template 
> > Array protobuf(const google::protobuf::RepeatedPtrField& repeated,
> > const lambda::function(const T&)>& converter)
> > {
> >   static_assert(std::is_convertible::value,
> > "T must be a google::protobuf::Message");
> >   JSON::Array array;
> >   array.values.reserve(repeated.size());
> >   foreach (const T& elem, repeated) {
> > array.values.push_back(converter(elem));
> >   }
> >   
> >   return array;
> > }
> > ```
> > 
> > Then the snippet above could be rewritten as:
> > ```
> > object.values["network_infos"] = 
> > std::move(JSON::protobuf(status.network_infos(), [](const NetworkInfo& 
> > info) { return model(info); });
> > ```
> > 
> > A further improvement would be to accept any iterable collection, not only 
> > `RepeatedPtrField<>`, for example `hashset`.
> > 
> > What do you think?
> 
> Klaus Ma wrote:
> Awesome! I've also try similar proposal, but failed when `function` 
> converting with `template`; your suggestion using lambda is great!
> For the `hashset`, I'd suggest to address it when we have such case in 
> our code :).
> 
> I'll also address your comments above.
> 
> Alexander Rukletsov wrote:
> We do have such cases in our codebase ; ). Here are a few as an example:
> - https://github.com/apache/mesos/blob/master/src/master/http.cpp#L217
> - https://github.com/apache/mesos/blob/master/src/master/http.cpp#L229

I like the idea of taking a projection function as an argument, but let's do it 
as a separate ticket to keep the scope of this review narrow.


- Michael


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


On Sept. 27, 2015, 1:34 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Sept. 27, 2015, 1:34 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 
> 
> Diff: https://reviews.apache.org/r/38342/diff/
> 
> 
> Testing
> ---
> 
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-09-29 Thread Alexander Rukletsov

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

3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (line 621)

Not yours, but while you're editing here, can you `.reserve()` please?

3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (line 638)

ditto

3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (lines 760 - 761)

I've seen your discussion with @Jan above, here is what I think regarding 
`ABORT` vs. `Try<>`. 

First off, some background. We tend to use `ABORT()` and `CHECK_*` macros 
when so-called "internal invariant" is violated, for example an object is being 
used without proper initialization, or a change is being made to an instance, 
that does not exists any more. On the other side `Try<>` and `Error<>` family 
is used when our expectation about the outer world does not hold, or, in other 
words, when an action cannot be completed, but no internal invariant is broken. 
User input, I/O, network operations are good examples of the latter case.

Let's try to figure out what we have here. I would say, it's an internal 
invariant, and here is why. JSON is less strict as Protobuf, therefore 
conversion Protobuf->JSON always exists (note that this is not symmetrical, 
JSON->Protobuf is not always possible, hence we use `Try<>` in e.g. 
`protobuf::parse<>()`). The only reason it may fail (remember we do not handle 
OOM exceptions) is because we convert a protobuf message of a yet unknown 
format (most probably future proto versions), which is a violation of an 
internal invariant!

Therefore I would suggest we keep `ABORT()` but document in the preambular 
comment why we use `ABORT()` and not `Try<>`, explaining what internal 
invariant is in this case.

- Alexander Rukletsov

On Sept. 27, 2015, 1:34 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Sept. 27, 2015, 1:34 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 
> 
> Diff: https://reviews.apache.org/r/38342/diff/
> 
> 
> Testing
> ---
> 
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-09-29 Thread Alexander Rukletsov


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



3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (line 776)


How about `.emplace_back()` here?


- Alexander Rukletsov


On Sept. 27, 2015, 1:34 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Sept. 27, 2015, 1:34 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 
> 
> Diff: https://reviews.apache.org/r/38342/diff/
> 
> 
> Testing
> ---
> 
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-09-29 Thread Alexander Rukletsov


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


One high level suggestion.

After looking at our http code, I realized that we use the same pattern again 
and again, for example:
```
JSON::Array array;
array.values.reserve(status.network_infos().size()); // MESOS-2353.
foreach (const NetworkInfo& info, status.network_infos()) {
  array.values.push_back(model(info));
}
object.values["network_infos"] = std::move(array);
```
We cannot use newly added `JSON::protobuf()` here, because a different way for 
rendering JSON from protobuf is used. Without digging deep inside, I know three 
ways how we create a `JSON` out of a proto in our codebase:
- wrap in `JSON::Protobuf()` for individual messages;
- wrap in one of the `model()` family functions;
- pass as it is for built-in types.

The proposed conversion function covers one of the possible ways. How about add 
one more convertion? Something like:
```
template 
Array protobuf(const google::protobuf::RepeatedPtrField& repeated,
const lambda::function(const T&)>& converter)
{
  static_assert(std::is_convertible::value,
"T must be a google::protobuf::Message");
  JSON::Array array;
  array.values.reserve(repeated.size());
  foreach (const T& elem, repeated) {
array.values.push_back(converter(elem));
  }
  
  return array;
}
```

Then the snippet above could be rewritten as:
```
object.values["network_infos"] = 
std::move(JSON::protobuf(status.network_infos(), [](const NetworkInfo& info) { 
return model(info); });
```

A further improvement would be to accept any iterable collection, not only 
`RepeatedPtrField<>`, for example `hashset`.

What do you think?

- Alexander Rukletsov


On Sept. 27, 2015, 1:34 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Sept. 27, 2015, 1:34 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 
> 
> Diff: https://reviews.apache.org/r/38342/diff/
> 
> 
> Testing
> ---
> 
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-09-29 Thread Klaus Ma



> On Sept. 29, 2015, 10:43 a.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h, line 40
> > 
> >
> > Just to confirm: this file is generated using bundled protobuf, right? 
> > Could you please put the `protoc` version in the RR description for 
> > posterity?

Yes, I used `protoc` built from our source code.


> On Sept. 29, 2015, 10:43 a.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto, line 80
> > 
> >
> > s/MessageArray/ArrayMessage
> > 
> > A high level question: why wouldn't you use `SimpleMessage` instead? 
> > Proto->JSON conversion for a single message is already checked in the 
> > different test, you want to check the conversion for collections. I would 
> > say, using `SimpleMessage` can reduce the amount of test code needed.

Agree, it'll reduce the LOC (line of code).


> On Sept. 29, 2015, 10:43 a.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, lines 773-774
> > 
> >
> > Could you please help me understand why this check is necessary? IIUC, 
> > compiler ensures `T` is a protobuf becasue you pass `elem` of type `T` to 
> > `JSON::protobuf()`.
> > 
> > AFAIK, the only reason to do this check is to prohibit arguments like 
> > `google::protobuf::RepeatedPtrField`.
> >  Do you want to express this? I don't think it is necessary, because 
> > `JSON::Array` filled with `JSON::Array` is fine.
> > 
> > @MPark, what'd you say?

To me, this's a safe guard to make sure this function is not abused.
For 
`google::protobuf::RepeatedPtrField`, I 
think we need to define a message to include 
`google::protobuf::RepeatedPtrField`, the proto file looks like:

message MyMessage
{
message Item {
string value = 1;
}
repeated Item values = 1;
}


- Klaus


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


On Sept. 27, 2015, 1:34 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Sept. 27, 2015, 1:34 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 
> 
> Diff: https://reviews.apache.org/r/38342/diff/
> 
> 
> Testing
> ---
> 
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-09-28 Thread Klaus Ma



> On Sept. 28, 2015, 9:11 a.m., Jan Schlicht wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, line 762
> > 
> >
> > Because this code has been changed from a constructor to a function, we 
> > can do better than calling `ABORT` here. We can change the function 
> > signature to return a `Try` and call `Error` here instead of 
> > `ABORT`. For example see the `write` function in line 55.
> > 
> > The function signature would change to
> > `inline Try protobuf(const google::protobuf::Message& message)`
> 
> Klaus Ma wrote:
> Got your point :).
> Just one concern on how to use it? We did not handle its result if failed 
> in `src`; personally, this's a kind of code practice to avoid undefined 
> protobuf field type. Anyway, I'll update accordingly if necessary.
> 
> Jan Schlicht wrote:
> Well, unfortunately this change means that the code that is using this 
> function has to be changed to handle the error cases. This means checking if 
> the `Try<>` contains a value or an error and react accordingly.

Did you check the code diff of #38335? In #38335, any suggestion on the 
function that return void; those function did not expect json parsing will 
fail. Personally, it's a bit overkill to re-implement them for json paring 
error handling which are not expect failed. 

@alex-mesos, @mcypark, what's your suggestion?


- Klaus


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


On Sept. 27, 2015, 1:34 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Sept. 27, 2015, 1:34 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 
> 
> Diff: https://reviews.apache.org/r/38342/diff/
> 
> 
> Testing
> ---
> 
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-09-28 Thread Jan Schlicht



> On Sept. 25, 2015, 3:35 p.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, line 609
> > 
> >
> > 1. You can also use `inline`, that's what we usually do.
> > 2. `static_assert` can be replaced by pulling the type into the 
> > fucntion signature, not by `Try<>`.
> > 3. What I think Jan means instead is that instead of calling an 
> > `ABORT`, why not using a `Try<>`?
> 
> Klaus Ma wrote:
> 1. Yes, `inline` is fine according to the document; i'll have a try 
> (seems enhanced in C++11 :) ).
> 2. `static_assert` will issue a **compile-time** error if test failed, 
> `Try<>` will check the result at **run-time**; so prefer to `static_assert` 
> for `Array protobuf(...)`.

Changes are looking good! My apologies, I've made the mistake of addressing to 
seperate issues in one issue... The `Try<>` stuff doesn't relate to the 
`static_assert`. Let me try again at a diffent position :)


- Jan


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


On Sept. 27, 2015, 3:34 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Sept. 27, 2015, 3:34 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 
> 
> Diff: https://reviews.apache.org/r/38342/diff/
> 
> 
> Testing
> ---
> 
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-09-28 Thread Klaus Ma



> On Sept. 28, 2015, 9:11 a.m., Jan Schlicht wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, line 762
> > 
> >
> > Because this code has been changed from a constructor to a function, we 
> > can do better than calling `ABORT` here. We can change the function 
> > signature to return a `Try` and call `Error` here instead of 
> > `ABORT`. For example see the `write` function in line 55.
> > 
> > The function signature would change to
> > `inline Try protobuf(const google::protobuf::Message& message)`

Got your point :).
Just one concern on how to use it? We did not handle its result if failed in 
`src`; personally, this's a kind of code practice to avoid undefined protobuf 
field type. Anyway, I'll update accordingly if necessary.


- Klaus


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


On Sept. 27, 2015, 1:34 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Sept. 27, 2015, 1:34 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 
> 
> Diff: https://reviews.apache.org/r/38342/diff/
> 
> 
> Testing
> ---
> 
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-09-28 Thread Jan Schlicht



> On Sept. 28, 2015, 11:11 a.m., Jan Schlicht wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, line 762
> > 
> >
> > Because this code has been changed from a constructor to a function, we 
> > can do better than calling `ABORT` here. We can change the function 
> > signature to return a `Try` and call `Error` here instead of 
> > `ABORT`. For example see the `write` function in line 55.
> > 
> > The function signature would change to
> > `inline Try protobuf(const google::protobuf::Message& message)`
> 
> Klaus Ma wrote:
> Got your point :).
> Just one concern on how to use it? We did not handle its result if failed 
> in `src`; personally, this's a kind of code practice to avoid undefined 
> protobuf field type. Anyway, I'll update accordingly if necessary.

Well, unfortunately this change means that the code that is using this function 
has to be changed to handle the error cases. This means checking if the `Try<>` 
contains a value or an error and react accordingly.


- Jan


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


On Sept. 27, 2015, 3:34 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Sept. 27, 2015, 3:34 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 
> 
> Diff: https://reviews.apache.org/r/38342/diff/
> 
> 
> Testing
> ---
> 
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-09-26 Thread Klaus Ma


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

(Updated Sept. 27, 2015, 1:34 a.m.)


Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.


Changes
---

Address comments:
1. Add inline to `Object protobuf(const google::protobuf::Message& message)`
2. keep `static_assert` to check error at compiling time
3. move the commnets outside the function


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


Repository: mesos


Description
---

Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
converts a `google::protobuf::Message` into a `JSON::Object`.
We should add the support for `google::protobuf::RepeatedPtrField` by 
introducing overloaded functions.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 

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


Testing
---

cd 3rdparty/libprocess/3rdparty/stout
./boostrap
./configure
make


Thanks,

Klaus Ma

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-09-25 Thread Klaus Ma



> On Sept. 25, 2015, 1:35 p.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, line 609
> > 
> >
> > 1. You can also use `inline`, that's what we usually do.
> > 2. `static_assert` can be replaced by pulling the type into the 
> > fucntion signature, not by `Try<>`.
> > 3. What I think Jan means instead is that instead of calling an 
> > `ABORT`, why not using a `Try<>`?

1. Yes, `inline` is fine according to the document; i'll have a try (seems 
enhanced in C++11 :) ).
2. `static_assert` will issue a **compile-time** error if test failed, `Try<>` 
will check the result at **run-time**; so prefer to `static_assert` for `Array 
protobuf(...)`.


- Klaus


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


On Sept. 24, 2015, 3 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Sept. 24, 2015, 3 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 
> 
> Diff: https://reviews.apache.org/r/38342/diff/
> 
> 
> Testing
> ---
> 
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-09-25 Thread Alexander Rukletsov


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



3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (line 609)


1. You can also use `inline`, that's what we usually do.
2. `static_assert` can be replaced by pulling the type into the fucntion 
signature, not by `Try<>`.
3. What I think Jan means instead is that instead of calling an `ABORT`, 
why not using a `Try<>`?


- Alexander Rukletsov


On Sept. 24, 2015, 3 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Sept. 24, 2015, 3 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 
> 
> Diff: https://reviews.apache.org/r/38342/diff/
> 
> 
> Testing
> ---
> 
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-09-24 Thread Klaus Ma


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

(Updated Sept. 24, 2015, 2:37 p.m.)


Review request for mesos, Alexander Rukletsov and Michael Park.


Changes
---

Resolved code conflict


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


Repository: mesos


Description
---

Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
converts a `google::protobuf::Message` into a `JSON::Object`.
We should add the support for `google::protobuf::RepeatedPtrField` by 
introducing overloaded functions.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 

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


Testing
---

cd 3rdparty/libprocess/3rdparty/stout
./boostrap
./configure
make


Thanks,

Klaus Ma

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-09-24 Thread Klaus Ma



> On Sept. 15, 2015, 12:59 p.m., Jan Schlicht wrote:
> > Please rebase your patch, latest commits in master lots of JSON related 
> > stuff.
> 
> Jan Schlicht wrote:
> Will review this RR after rebase.

The code diff was uploaded after resolving the conflict


- Klaus


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


On Sept. 24, 2015, 2:37 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Sept. 24, 2015, 2:37 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Michael Park.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 
> 
> Diff: https://reviews.apache.org/r/38342/diff/
> 
> 
> Testing
> ---
> 
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-09-24 Thread Jan Schlicht


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



3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (line 609)


s/protobuf(const T& message)/protobuf(const google::protobuf::Message& 
message)

The templatization in 608 isn't necessary here, as the static_assert in 611 
has the same effect as just using a google::protobuf::Message as function 
parameter. Please change the function signature and remove line 608 & 611,612.

Also, it'd make sense to return a `Try` as parsing `message` may 
fail. The current behavior is to call ABORT, one could as well return `None` 
here.



3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (line 615)


This comment describes a behavior of the function and should be moved to 
608 (= above the function signature).



3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (line 772)


Same as before: It'd make sense to return a `Try` here.


- Jan Schlicht


On Sept. 24, 2015, 5 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Sept. 24, 2015, 5 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 
> 
> Diff: https://reviews.apache.org/r/38342/diff/
> 
> 
> Testing
> ---
> 
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-09-24 Thread Klaus Ma



> On Sept. 24, 2015, 3:52 p.m., Jan Schlicht wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, line 609
> > 
> >
> > s/protobuf(const T& message)/protobuf(const google::protobuf::Message& 
> > message)
> > 
> > The templatization in 608 isn't necessary here, as the static_assert in 
> > 611 has the same effect as just using a google::protobuf::Message as 
> > function parameter. Please change the function signature and remove line 
> > 608 & 611,612.
> > 
> > Also, it'd make sense to return a `Try` as parsing `message` 
> > may fail. The current behavior is to call ABORT, one could as well return 
> > `None` here.

Used template here because there's a link error if we used `protobuf(const 
google::protobuf::Message& message)`; the way to resovle is to move 
implementation into cpp file, but stout is a library only including headers.

`static_assert` will generate error when compiling, it will check error before 
running; so we can use `Object` directly :).


> On Sept. 24, 2015, 3:52 p.m., Jan Schlicht wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, line 774
> > 
> >
> > Same as before: It'd make sense to return a `Try` here.

Same answer to `Object` :).


> On Sept. 24, 2015, 3:52 p.m., Jan Schlicht wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, line 615
> > 
> >
> > This comment describes a behavior of the function and should be moved 
> > to 608 (= above the function signature).

I'll address it :).


- Klaus


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


On Sept. 24, 2015, 3 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Sept. 24, 2015, 3 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 2285ce9 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 68328a2 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 8ebb798 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 34eb6d0 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 920f5c9 
> 
> Diff: https://reviews.apache.org/r/38342/diff/
> 
> 
> Testing
> ---
> 
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-09-15 Thread Klaus Ma


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

(Updated Sept. 15, 2015, 9:43 a.m.)


Review request for mesos, Alexander Rukletsov and Michael Park.


Changes
---

Address comments


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


Repository: mesos


Description
---

Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
converts a `google::protobuf::Message` into a `JSON::Object`.
We should add the support for `google::protobuf::RepeatedPtrField` by 
introducing overloaded functions.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp f28138c 
  3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf 
  3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 850650c 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp c56d6a3 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h cfc2803 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc a1d4084 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto bbd36d3 

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


Testing
---

cd 3rdparty/libprocess/3rdparty/stout
./boostrap
./configure
make


Thanks,

Klaus Ma

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-09-15 Thread Klaus Ma



> On Sept. 14, 2015, 5:39 p.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp, line 112
> > 
> >
> > Let's `reserve()` first.

Addressed, thanks :).


- Klaus


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


On Sept. 15, 2015, 9:43 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Sept. 15, 2015, 9:43 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Michael Park.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp f28138c 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf 
>   3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 850650c 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp c56d6a3 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h cfc2803 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc a1d4084 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto bbd36d3 
> 
> Diff: https://reviews.apache.org/r/38342/diff/
> 
> 
> Testing
> ---
> 
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-09-15 Thread Jan Schlicht



> On Sept. 15, 2015, 2:59 p.m., Jan Schlicht wrote:
> > Please rebase your patch, latest commits in master lots of JSON related 
> > stuff.

Will review this RR after rebase.


- Jan


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


On Sept. 15, 2015, 11:43 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Sept. 15, 2015, 11:43 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Michael Park.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp f28138c 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf 
>   3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 850650c 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp c56d6a3 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h cfc2803 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc a1d4084 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto bbd36d3 
> 
> Diff: https://reviews.apache.org/r/38342/diff/
> 
> 
> Testing
> ---
> 
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38342: Add JSON::protobuf for google::protobuf::RepeatedPtrField (stout part)

2015-09-15 Thread Jan Schlicht


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


Please rebase your patch, latest commits in master lots of JSON related stuff.

- Jan Schlicht


On Sept. 15, 2015, 11:43 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38342/
> ---
> 
> (Updated Sept. 15, 2015, 11:43 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Michael Park.
> 
> 
> Bugs: MESOS-3405
> https://issues.apache.org/jira/browse/MESOS-3405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, `stout/protobuf.hpp` provides a `JSON::Protobuf` utility which 
> converts a `google::protobuf::Message` into a `JSON::Object`.
> We should add the support for `google::protobuf::RepeatedPtrField` by 
> introducing overloaded functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp f28138c 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 57d5fdf 
>   3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp 850650c 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp c56d6a3 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h cfc2803 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc a1d4084 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto bbd36d3 
> 
> Diff: https://reviews.apache.org/r/38342/diff/
> 
> 
> Testing
> ---
> 
> cd 3rdparty/libprocess/3rdparty/stout
> ./boostrap
> ./configure
> make
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

43 matches

Mail list logo