Re: Review Request 59987: Added protobuf map support to stout JSON<->protobuf conversion.

[Chun-Hung Hsiao]

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


Ship it!




Ship It!

- Chun-Hung Hsiao


On March 13, 2018, 9:13 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59987/
> ---
> 
> (Updated March 13, 2018, 9:13 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Benjamin Mahler, Chun-Hung Hsiao, 
> and Zhitao Li.
> 
> 
> Bugs: MESOS-7656
> https://issues.apache.org/jira/browse/MESOS-7656
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Map is a feature of proto2 syntax, but it can only be compiled
> with 3.0.0+ protobuf compiler, see the following discussion for
> details:
> 
> https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4
> 
> We have already upgraded the compiler from 2.6.1 to 3.3.0 in
> MESOS-7228. This patch adds map support in the json <-> protobuf
> conversion in stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/protobuf.hpp 
> 4a1605e5130dbf7e8286dbb43d0d04ab4394e79a 
> 
> 
> Diff: https://reviews.apache.org/r/59987/diff/9/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 59987: Added protobuf map support to stout JSON<->protobuf conversion.

[Qian Zhang]

> On March 13, 2018, 10:07 a.m., Chun-Hung Hsiao wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Lines 807-810 (patched)
> > 
> >
> > How about making this a lambda inside `protobuf()` so we don't expose 
> > this?

Agree!


- Qian


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


On March 13, 2018, 5:13 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59987/
> ---
> 
> (Updated March 13, 2018, 5:13 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Benjamin Mahler, Chun-Hung Hsiao, 
> and Zhitao Li.
> 
> 
> Bugs: MESOS-7656
> https://issues.apache.org/jira/browse/MESOS-7656
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Map is a feature of proto2 syntax, but it can only be compiled
> with 3.0.0+ protobuf compiler, see the following discussion for
> details:
> 
> https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4
> 
> We have already upgraded the compiler from 2.6.1 to 3.3.0 in
> MESOS-7228. This patch adds map support in the json <-> protobuf
> conversion in stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/protobuf.hpp 
> 4a1605e5130dbf7e8286dbb43d0d04ab4394e79a 
> 
> 
> Diff: https://reviews.apache.org/r/59987/diff/9/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 59987: Added protobuf map support to stout JSON<->protobuf conversion.

[Qian Zhang]

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

(Updated March 13, 2018, 5:13 p.m.)


Review request for mesos, Anand Mazumdar, Benjamin Mahler, Chun-Hung Hsiao, and 
Zhitao Li.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

Map is a feature of proto2 syntax, but it can only be compiled
with 3.0.0+ protobuf compiler, see the following discussion for
details:

https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4

We have already upgraded the compiler from 2.6.1 to 3.3.0 in
MESOS-7228. This patch adds map support in the json <-> protobuf
conversion in stout.


Diffs (updated)
-

  3rdparty/stout/include/stout/protobuf.hpp 
4a1605e5130dbf7e8286dbb43d0d04ab4394e79a 


Diff: https://reviews.apache.org/r/59987/diff/9/

Changes: https://reviews.apache.org/r/59987/diff/8-9/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 59987: Added protobuf map support to stout JSON<->protobuf conversion.

[Chun-Hung Hsiao]

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




3rdparty/stout/include/stout/protobuf.hpp
Lines 807-810 (patched)


How about making this a lambda inside `protobuf()` so we don't expose this?



3rdparty/stout/include/stout/protobuf.hpp
Lines 1018-1059 (patched)


How about:
```
JSON::Value key = value_for_field(entry, key_field);

if (key.is()) {
  name = key.as().value;
} else {
  name = jsonify(key);
}
```


- Chun-Hung Hsiao


On March 12, 2018, 7:15 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59987/
> ---
> 
> (Updated March 12, 2018, 7:15 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Benjamin Mahler, Chun-Hung Hsiao, 
> and Zhitao Li.
> 
> 
> Bugs: MESOS-7656
> https://issues.apache.org/jira/browse/MESOS-7656
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Map is a feature of proto2 syntax, but it can only be compiled
> with 3.0.0+ protobuf compiler, see the following discussion for
> details:
> 
> https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4
> 
> We have already upgraded the compiler from 2.6.1 to 3.3.0 in
> MESOS-7228. This patch adds map support in the json <-> protobuf
> conversion in stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/protobuf.hpp 
> 4a1605e5130dbf7e8286dbb43d0d04ab4394e79a 
> 
> 
> Diff: https://reviews.apache.org/r/59987/diff/8/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 59987: Added protobuf map support to stout JSON<->protobuf conversion.

[Qian Zhang]

> On March 1, 2018, 12:10 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Lines 391-487 (patched)
> > 
> >
> > It looks like we can avoid this logic since protobuf's JSON conversion 
> > allows the protobuf key types (bool, integers, strings) to be converted 
> > from JSON strings. This means we could just recurse for both key and value 
> > here:
> > 
> > ```
> > // Some comment explaining what map is equivalent to with
> > // a reference to the google documentation.
> > google::protobuf::Message* entry =
> >   reflection->AddMessage(message, field);
> > 
> > const google::protobuf::FieldDescriptor* key_field =
> >   entry->GetDescriptor()->FindFieldByNumber(1);
> >   
> > // Maybe passing just 'key' works due to implicit 
> > construction?
> > // TODO(...): This copies the key, consider avoiding this.
> > Try apply =
> >   boost::apply_visitor(Parser(entry, key_field), 
> > JSON::String(key));
> > 
> > if (apply.isError()) {
> >   return Error(apply.error());
> > }
> >   
> > const google::protobuf::FieldDescriptor* value_field =
> >   entry->GetDescriptor()->FindFieldByNumber(2);
> >   
> > Try apply =
> >   boost::apply_visitor(Parser(entry, value_field), value);
> > 
> > if (apply.isError()) {
> >   return Error(apply.error());
> > }
> > ```
> > 
> > Now, to make this simplification work, we need to update our JSON 
> > conversion in a separate patch to allow bools and integers to be parsed 
> > from JSON strings to match google's logic for conversion:
> > 
> > 
> > https://github.com/google/protobuf/blob/v3.5.1/src/google/protobuf/util/internal/datapiece.cc#L203
> > 
> > Documentation here: 
> > https://developers.google.com/protocol-buffers/docs/proto3#json
> 
> Qian Zhang wrote:
> > // Maybe passing just 'key' works due to implicit construction?
> 
> We cannot pass `key` or even `JSON::String` to `boost::apply_visitor()` 
> because it cannot pass compilation:
> ```
> 
> ../3rdparty/boost-1.65.0/boost/variant/detail/apply_visitor_unary.hpp:84:22: 
> error: no member named 'apply_visitor' in 'std::__1::basic_string'
> ```
> or
> ```
> 
> ../3rdparty/boost-1.65.0/boost/variant/detail/apply_visitor_unary.hpp:84:22: 
> error: no member named 'apply_visitor' in 'JSON::String'
> ```
> That is because `std::string` and `JSON::String` do not have a member 
> `apply_visitor`, so I think we have to use `JSON::Value` like this:
> ```
> google::protobuf::Message* entry =
>   reflection->AddMessage(message, field);
> 
> const google::protobuf::FieldDescriptor* key_field =
>   entry->GetDescriptor()->FindFieldByNumber(1);
> 
> JSON::Value key(name);
> 
> Try apply =
>   boost::apply_visitor(Parser(entry, key_field), key);
> 
> if (apply.isError()) {
>   return Error(apply.error());
> }
> 
> const google::protobuf::FieldDescriptor* value_field =
>   entry->GetDescriptor()->FindFieldByNumber(2);
> 
> apply = boost::apply_visitor(Parser(entry, value_field), 
> value);
> if (apply.isError()) {
>   return Error(apply.error());
> }
> ```
> 
> > we need to update our JSON conversion in a separate patch to allow 
> bools and integers to be parsed from JSON strings to match google's logic for 
> conversion
> 
> Did you mean we should improve the method below by adding more cases for 
> converting `JSON::String` to bools and integers?
> ```
>   Try operator()(const JSON::String& string) const
>   {
> switch (field->type()) {
>   case google::protobuf::FieldDescriptor::TYPE_STRING:
>   ...
> ```
> If so, then that means moving the code here (see below) into the above 
> method. But I think those code are specific for map support, however 
> `Try operator()(const JSON::String& string) const` is a generic 
> method for JSON string conversion, so I would still like to keep those code 
> as where they are now in this patch (i.e., the code path to handle map).
> ```
>   case google::protobuf::FieldDescriptor::TYPE_INT64:
>   case google::protobuf::FieldDescriptor::TYPE_SINT64:
>   case google::protobuf::FieldDescriptor::TYPE_SFIXED64:
>   {
> 

Re: Review Request 59987: Added protobuf map support to stout JSON<->protobuf conversion.

[Qian Zhang]

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

(Updated March 12, 2018, 3:15 p.m.)


Review request for mesos, Anand Mazumdar, Benjamin Mahler, Chun-Hung Hsiao, and 
Zhitao Li.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

Map is a feature of proto2 syntax, but it can only be compiled
with 3.0.0+ protobuf compiler, see the following discussion for
details:

https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4

We have already upgraded the compiler from 2.6.1 to 3.3.0 in
MESOS-7228. This patch adds map support in the json <-> protobuf
conversion in stout.


Diffs (updated)
-

  3rdparty/stout/include/stout/protobuf.hpp 
4a1605e5130dbf7e8286dbb43d0d04ab4394e79a 


Diff: https://reviews.apache.org/r/59987/diff/8/

Changes: https://reviews.apache.org/r/59987/diff/7-8/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 59987: Added protobuf map support to stout JSON<->protobuf conversion.

[Qian Zhang]

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

(Updated March 9, 2018, 10:08 p.m.)


Review request for mesos, Anand Mazumdar, Benjamin Mahler, Chun-Hung Hsiao, and 
Zhitao Li.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

Map is a feature of proto2 syntax, but it can only be compiled
with 3.0.0+ protobuf compiler, see the following discussion for
details:

https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4

We have already upgraded the compiler from 2.6.1 to 3.3.0 in
MESOS-7228. This patch adds map support in the json <-> protobuf
conversion in stout.


Diffs (updated)
-

  3rdparty/stout/include/stout/protobuf.hpp 
4a1605e5130dbf7e8286dbb43d0d04ab4394e79a 


Diff: https://reviews.apache.org/r/59987/diff/7/

Changes: https://reviews.apache.org/r/59987/diff/6-7/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 59987: Added protobuf map support to stout JSON<->protobuf conversion.

[Qian Zhang]

> On March 1, 2018, 12:10 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Lines 391-487 (patched)
> > 
> >
> > It looks like we can avoid this logic since protobuf's JSON conversion 
> > allows the protobuf key types (bool, integers, strings) to be converted 
> > from JSON strings. This means we could just recurse for both key and value 
> > here:
> > 
> > ```
> > // Some comment explaining what map is equivalent to with
> > // a reference to the google documentation.
> > google::protobuf::Message* entry =
> >   reflection->AddMessage(message, field);
> > 
> > const google::protobuf::FieldDescriptor* key_field =
> >   entry->GetDescriptor()->FindFieldByNumber(1);
> >   
> > // Maybe passing just 'key' works due to implicit 
> > construction?
> > // TODO(...): This copies the key, consider avoiding this.
> > Try apply =
> >   boost::apply_visitor(Parser(entry, key_field), 
> > JSON::String(key));
> > 
> > if (apply.isError()) {
> >   return Error(apply.error());
> > }
> >   
> > const google::protobuf::FieldDescriptor* value_field =
> >   entry->GetDescriptor()->FindFieldByNumber(2);
> >   
> > Try apply =
> >   boost::apply_visitor(Parser(entry, value_field), value);
> > 
> > if (apply.isError()) {
> >   return Error(apply.error());
> > }
> > ```
> > 
> > Now, to make this simplification work, we need to update our JSON 
> > conversion in a separate patch to allow bools and integers to be parsed 
> > from JSON strings to match google's logic for conversion:
> > 
> > 
> > https://github.com/google/protobuf/blob/v3.5.1/src/google/protobuf/util/internal/datapiece.cc#L203
> > 
> > Documentation here: 
> > https://developers.google.com/protocol-buffers/docs/proto3#json
> 
> Qian Zhang wrote:
> > // Maybe passing just 'key' works due to implicit construction?
> 
> We cannot pass `key` or even `JSON::String` to `boost::apply_visitor()` 
> because it cannot pass compilation:
> ```
> 
> ../3rdparty/boost-1.65.0/boost/variant/detail/apply_visitor_unary.hpp:84:22: 
> error: no member named 'apply_visitor' in 'std::__1::basic_string'
> ```
> or
> ```
> 
> ../3rdparty/boost-1.65.0/boost/variant/detail/apply_visitor_unary.hpp:84:22: 
> error: no member named 'apply_visitor' in 'JSON::String'
> ```
> That is because `std::string` and `JSON::String` do not have a member 
> `apply_visitor`, so I think we have to use `JSON::Value` like this:
> ```
> google::protobuf::Message* entry =
>   reflection->AddMessage(message, field);
> 
> const google::protobuf::FieldDescriptor* key_field =
>   entry->GetDescriptor()->FindFieldByNumber(1);
> 
> JSON::Value key(name);
> 
> Try apply =
>   boost::apply_visitor(Parser(entry, key_field), key);
> 
> if (apply.isError()) {
>   return Error(apply.error());
> }
> 
> const google::protobuf::FieldDescriptor* value_field =
>   entry->GetDescriptor()->FindFieldByNumber(2);
> 
> apply = boost::apply_visitor(Parser(entry, value_field), 
> value);
> if (apply.isError()) {
>   return Error(apply.error());
> }
> ```
> 
> > we need to update our JSON conversion in a separate patch to allow 
> bools and integers to be parsed from JSON strings to match google's logic for 
> conversion
> 
> Did you mean we should improve the method below by adding more cases for 
> converting `JSON::String` to bools and integers?
> ```
>   Try operator()(const JSON::String& string) const
>   {
> switch (field->type()) {
>   case google::protobuf::FieldDescriptor::TYPE_STRING:
>   ...
> ```
> If so, then that means moving the code here (see below) into the above 
> method. But I think those code are specific for map support, however 
> `Try operator()(const JSON::String& string) const` is a generic 
> method for JSON string conversion, so I would still like to keep those code 
> as where they are now in this patch (i.e., the code path to handle map).
> ```
>   case google::protobuf::FieldDescriptor::TYPE_INT64:
>   case google::protobuf::FieldDescriptor::TYPE_SINT64:
>   case google::protobuf::FieldDescriptor::TYPE_SFIXED64:
>   {
> 

Re: Review Request 59987: Added protobuf map support to stout JSON<->protobuf conversion.

[Chun-Hung Hsiao]

> On March 1, 2018, 4:10 a.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Lines 388 (patched)
> > 
> >
> > s/name/key/?
> 
> Qian Zhang wrote:
> I'd like to be consistent with the existing code: 
> https://github.com/apache/mesos/blob/1.5.0/3rdparty/stout/include/stout/protobuf.hpp#L575:L576.
>  And in another hand, in the code below, I already have another local 
> variable named `key`, e.g.,:
> ```
> Try key = numify(name);
> ```
> So I'd like to differentiate them.

I have no objection to keep the nameing as is. But on the other hand, we name 
it `name` probably because of `FindFieldByName(name)`. I'll leave this up to 
you.


> On March 1, 2018, 4:10 a.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Lines 391-487 (patched)
> > 
> >
> > It looks like we can avoid this logic since protobuf's JSON conversion 
> > allows the protobuf key types (bool, integers, strings) to be converted 
> > from JSON strings. This means we could just recurse for both key and value 
> > here:
> > 
> > ```
> > // Some comment explaining what map is equivalent to with
> > // a reference to the google documentation.
> > google::protobuf::Message* entry =
> >   reflection->AddMessage(message, field);
> > 
> > const google::protobuf::FieldDescriptor* key_field =
> >   entry->GetDescriptor()->FindFieldByNumber(1);
> >   
> > // Maybe passing just 'key' works due to implicit 
> > construction?
> > // TODO(...): This copies the key, consider avoiding this.
> > Try apply =
> >   boost::apply_visitor(Parser(entry, key_field), 
> > JSON::String(key));
> > 
> > if (apply.isError()) {
> >   return Error(apply.error());
> > }
> >   
> > const google::protobuf::FieldDescriptor* value_field =
> >   entry->GetDescriptor()->FindFieldByNumber(2);
> >   
> > Try apply =
> >   boost::apply_visitor(Parser(entry, value_field), value);
> > 
> > if (apply.isError()) {
> >   return Error(apply.error());
> > }
> > ```
> > 
> > Now, to make this simplification work, we need to update our JSON 
> > conversion in a separate patch to allow bools and integers to be parsed 
> > from JSON strings to match google's logic for conversion:
> > 
> > 
> > https://github.com/google/protobuf/blob/v3.5.1/src/google/protobuf/util/internal/datapiece.cc#L203
> > 
> > Documentation here: 
> > https://developers.google.com/protocol-buffers/docs/proto3#json
> 
> Qian Zhang wrote:
> > // Maybe passing just 'key' works due to implicit construction?
> 
> We cannot pass `key` or even `JSON::String` to `boost::apply_visitor()` 
> because it cannot pass compilation:
> ```
> 
> ../3rdparty/boost-1.65.0/boost/variant/detail/apply_visitor_unary.hpp:84:22: 
> error: no member named 'apply_visitor' in 'std::__1::basic_string'
> ```
> or
> ```
> 
> ../3rdparty/boost-1.65.0/boost/variant/detail/apply_visitor_unary.hpp:84:22: 
> error: no member named 'apply_visitor' in 'JSON::String'
> ```
> That is because `std::string` and `JSON::String` do not have a member 
> `apply_visitor`, so I think we have to use `JSON::Value` like this:
> ```
> google::protobuf::Message* entry =
>   reflection->AddMessage(message, field);
> 
> const google::protobuf::FieldDescriptor* key_field =
>   entry->GetDescriptor()->FindFieldByNumber(1);
> 
> JSON::Value key(name);
> 
> Try apply =
>   boost::apply_visitor(Parser(entry, key_field), key);
> 
> if (apply.isError()) {
>   return Error(apply.error());
> }
> 
> const google::protobuf::FieldDescriptor* value_field =
>   entry->GetDescriptor()->FindFieldByNumber(2);
> 
> apply = boost::apply_visitor(Parser(entry, value_field), 
> value);
> if (apply.isError()) {
>   return Error(apply.error());
> }
> ```
> 
> > we need to update our JSON conversion in a separate patch to allow 
> bools and integers to be parsed from JSON strings to match google's logic for 
> conversion
> 
> Did you mean we should improve the method below by adding more cases for 
> converting `JSON::String` to bools and integers?
> ```
>   Try operator()(const JSON::String& string) const
>   {

Re: Review Request 59987: Added protobuf map support to stout JSON<->protobuf conversion.

[Qian Zhang]

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

(Updated March 6, 2018, 5:29 p.m.)


Review request for mesos, Anand Mazumdar, Benjamin Mahler, Chun-Hung Hsiao, and 
Zhitao Li.


Changes
---

Addressed review comments.


Summary (updated)
-

Added protobuf map support to stout JSON<->protobuf conversion.


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


Repository: mesos


Description (updated)
---

Map is a feature of proto2 syntax, but it can only be compiled
with 3.0.0+ protobuf compiler, see the following discussion for
details:

https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4

We have already upgraded the compiler from 2.6.1 to 3.3.0 in
MESOS-7228. This patch adds map support in the json <-> protobuf
conversion in stout.


Diffs (updated)
-

  3rdparty/stout/include/stout/protobuf.hpp 
4a1605e5130dbf7e8286dbb43d0d04ab4394e79a 


Diff: https://reviews.apache.org/r/59987/diff/6/

Changes: https://reviews.apache.org/r/59987/diff/5-6/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 59987: Added protobuf map support.

[Qian Zhang]

> On March 1, 2018, 12:10 p.m., Benjamin Mahler wrote:
> > Chun and I went over this together, so feel free to reach out to either of 
> > us for discussion! Just some suggestions to clean up the code below.
> > 
> > In the description, could we mention that this is for stout's json <-> 
> > protobuf conversion? E.g.
> > 
> > ```
> > Added protobuf map support to stout JSON<->protobuf conversion.
> > 
> > Map is a feature of proto2 syntax, but it can only be compiled
> > with 3.0.0+ protobuf compiler, see the following discussion for
> > details:
> >   
> >   https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4
> > 
> > We have already upgraded the compiler from 2.6.1 to 3.3.0 in
> > MESOS-7228. This patch adds map support in the json <-> protobuf
> > conversion in stout.
> > ```

Sure, I have updated the commit message to mention it.


> On March 1, 2018, 12:10 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Lines 32 (patched)
> > 
> >
> > FWICT, we're just supposed to include message.h for reflection:
> > 
> > 
> > https://developers.google.com/protocol-buffers/docs/reference/cpp/google.protobuf.message#Reflection
> > 
> > And this reflection.h header seems internal and already included by 
> > message.h? It only seems to define some pieces of the reflection code:
> > 
> > 
> > https://github.com/google/protobuf/blob/v3.5.0/src/google/protobuf/reflection.h

You are right! Previous I called `reflection->GetRepeatedFieldRef()` which 
needs `reflection.h`:
https://github.com/google/protobuf/blob/v3.5.0/src/google/protobuf/message.h#L787:L792

But later I changed the solution by calling `reflection->AddMessage()`, so I 
should have removed it from the code, thanks for catching this!


> On March 1, 2018, 12:10 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Lines 388 (patched)
> > 
> >
> > s/name/key/?

I'd like to be consistent with the existing code: 
https://github.com/apache/mesos/blob/1.5.0/3rdparty/stout/include/stout/protobuf.hpp#L575:L576.
 And in another hand, in the code below, I already have another local variable 
named `key`, e.g.,:
```
Try key = numify(name);
```
So I'd like to differentiate them.


> On March 1, 2018, 12:10 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Lines 391-487 (patched)
> > 
> >
> > It looks like we can avoid this logic since protobuf's JSON conversion 
> > allows the protobuf key types (bool, integers, strings) to be converted 
> > from JSON strings. This means we could just recurse for both key and value 
> > here:
> > 
> > ```
> > // Some comment explaining what map is equivalent to with
> > // a reference to the google documentation.
> > google::protobuf::Message* entry =
> >   reflection->AddMessage(message, field);
> > 
> > const google::protobuf::FieldDescriptor* key_field =
> >   entry->GetDescriptor()->FindFieldByNumber(1);
> >   
> > // Maybe passing just 'key' works due to implicit 
> > construction?
> > // TODO(...): This copies the key, consider avoiding this.
> > Try apply =
> >   boost::apply_visitor(Parser(entry, key_field), 
> > JSON::String(key));
> > 
> > if (apply.isError()) {
> >   return Error(apply.error());
> > }
> >   
> > const google::protobuf::FieldDescriptor* value_field =
> >   entry->GetDescriptor()->FindFieldByNumber(2);
> >   
> > Try apply =
> >   boost::apply_visitor(Parser(entry, value_field), value);
> > 
> > if (apply.isError()) {
> >   return Error(apply.error());
> > }
> > ```
> > 
> > Now, to make this simplification work, we need to update our JSON 
> > conversion in a separate patch to allow bools and integers to be parsed 
> > from JSON strings to match google's logic for conversion:
> > 
> > 
> > https://github.com/google/protobuf/blob/v3.5.1/src/google/protobuf/util/internal/datapiece.cc#L203
> > 
> > Documentation here: 
> > https://developers.google.com/protocol-buffers/docs/proto3#json

> // Maybe passing just 'key' works due to implicit construction?

We cannot pass `key` or even `JSON::String` to `boost::apply_visitor()` because 
it cannot pass compilation:
```
../3rdparty/boost-1.65.0/boost/variant/detail/apply_visitor_unary.hpp:84:22: 
error: no member named 'apply_visitor' in 'std::__1::basic_string'
```
or
```
../3rdparty/boost-1.65.0/boost/variant

Re: Review Request 59987: Added protobuf map support.

[Benjamin Mahler]

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



Chun and I went over this together, so feel free to reach out to either of us 
for discussion! Just some suggestions to clean up the code below.

In the description, could we mention that this is for stout's json <-> protobuf 
conversion? E.g.

```
Added protobuf map support to stout JSON<->protobuf conversion.

Map is a feature of proto2 syntax, but it can only be compiled
with 3.0.0+ protobuf compiler, see the following discussion for
details:
  
  https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4

We have already upgraded the compiler from 2.6.1 to 3.3.0 in
MESOS-7228. This patch adds map support in the json <-> protobuf
conversion in stout.
```


3rdparty/stout/include/stout/protobuf.hpp
Lines 32 (patched)


FWICT, we're just supposed to include message.h for reflection:


https://developers.google.com/protocol-buffers/docs/reference/cpp/google.protobuf.message#Reflection

And this reflection.h header seems internal and already included by 
message.h? It only seems to define some pieces of the reflection code:


https://github.com/google/protobuf/blob/v3.5.0/src/google/protobuf/reflection.h



3rdparty/stout/include/stout/protobuf.hpp
Lines 388 (patched)


s/name/key/?



3rdparty/stout/include/stout/protobuf.hpp
Lines 391-487 (patched)


It looks like we can avoid this logic since protobuf's JSON conversion 
allows the protobuf key types (bool, integers, strings) to be converted from 
JSON strings. This means we could just recurse for both key and value here:

```
// Some comment explaining what map is equivalent to with
// a reference to the google documentation.
google::protobuf::Message* entry =
  reflection->AddMessage(message, field);

const google::protobuf::FieldDescriptor* key_field =
  entry->GetDescriptor()->FindFieldByNumber(1);
  
// Maybe passing just 'key' works due to implicit construction?
// TODO(...): This copies the key, consider avoiding this.
Try apply =
  boost::apply_visitor(Parser(entry, key_field), 
JSON::String(key));

if (apply.isError()) {
  return Error(apply.error());
}
  
const google::protobuf::FieldDescriptor* value_field =
  entry->GetDescriptor()->FindFieldByNumber(2);
  
Try apply =
  boost::apply_visitor(Parser(entry, value_field), value);

if (apply.isError()) {
  return Error(apply.error());
}
```

Now, to make this simplification work, we need to update our JSON 
conversion in a separate patch to allow bools and integers to be parsed from 
JSON strings to match google's logic for conversion:


https://github.com/google/protobuf/blob/v3.5.1/src/google/protobuf/util/internal/datapiece.cc#L203

Documentation here: 
https://developers.google.com/protocol-buffers/docs/proto3#json



3rdparty/stout/include/stout/protobuf.hpp
Lines 394-398 (patched)


Can we reference the documentation here and mention that a map is 
equivalent to:

```
message MapFieldEntry {
  optional key_type key = 1;
  optional value_type value = 2;
}

repeated MapFieldEntry map_field = N;
```



3rdparty/stout/include/stout/protobuf.hpp
Lines 400-401 (patched)


Can you move this out of the loop?



3rdparty/stout/include/stout/protobuf.hpp
Lines 1036-1037 (patched)


Google converts 64 bit integers into strings, it's pretty badly broken 
right now that we don't do this. I think we're going to have to switch to 
string and just deal with breaking any clients that assumed strings were not 
coming out. I don't know if you want to help fix this, but if you do I would be 
happy to review / discuss!



3rdparty/stout/include/stout/protobuf.hpp
Lines 1039-1043 (patched)


Is there a way to abstract out this logic into a lambda here and re-use it? 
E.g.

```
template 
JSON::Value value_for_field(
Message* entry,
google::protobuf::Reflection* reflection,
google::protobuf::FieldDescriptor* field)
{
  switch (type) {
case ...:
case TYPE_INT64:
  return

Re: Review Request 59987: Added protobuf map support.

[Chun-Hung Hsiao]

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


Ship it!




Ship It!

- Chun-Hung Hsiao


On Feb. 26, 2018, 9:05 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59987/
> ---
> 
> (Updated Feb. 26, 2018, 9:05 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Chun-Hung Hsiao, and Zhitao Li.
> 
> 
> Bugs: MESOS-7656
> https://issues.apache.org/jira/browse/MESOS-7656
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Map is a feature of proto2 syntax, but it can only be compiled
> with proto3 compiler, see the following discussion for details:
> https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4
> We have already upgraded the compiler from 2.6.1 to 3.3.0 in
> MESOS-7228, however, to use protobuf map in Mesos code, we also
> need to add the protobuf map support to the code in Mesos for
> converting protobuf message to JSON object and parsing JSON
> object as protobuf message, that is what I have done in this patch.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/protobuf.hpp 
> 4a1605e5130dbf7e8286dbb43d0d04ab4394e79a 
> 
> 
> Diff: https://reviews.apache.org/r/59987/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 59987: Added protobuf map support.

[Qian Zhang]

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

(Updated Feb. 26, 2018, 5:05 p.m.)


Review request for mesos, Anand Mazumdar, Chun-Hung Hsiao, and Zhitao Li.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

Map is a feature of proto2 syntax, but it can only be compiled
with proto3 compiler, see the following discussion for details:
https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4
We have already upgraded the compiler from 2.6.1 to 3.3.0 in
MESOS-7228, however, to use protobuf map in Mesos code, we also
need to add the protobuf map support to the code in Mesos for
converting protobuf message to JSON object and parsing JSON
object as protobuf message, that is what I have done in this patch.


Diffs (updated)
-

  3rdparty/stout/include/stout/protobuf.hpp 
4a1605e5130dbf7e8286dbb43d0d04ab4394e79a 


Diff: https://reviews.apache.org/r/59987/diff/5/

Changes: https://reviews.apache.org/r/59987/diff/4-5/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 59987: Added protobuf map support.

[Qian Zhang]

> On Feb. 9, 2018, 4:26 a.m., Chun-Hung Hsiao wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Lines 499-500 (patched)
> > 
> >
> > How about using `UNREACHABLE` instead?

With `ABORT`, we can output `key_field->type()` which will be helpful for 
debugging purpose, but we cannot do it with `UNREACHABLE`. So I would still 
prefer `ABORT`.


> On Feb. 9, 2018, 4:26 a.m., Chun-Hung Hsiao wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Lines 502-503 (patched)
> > 
> >
> > Deprecated or not, it cannot be the type of a key, so let's merge this 
> > case with the above cases.

Agree!


> On Feb. 9, 2018, 4:26 a.m., Chun-Hung Hsiao wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Lines 1045-1046 (patched)
> > 
> >
> > Ditto.

With `ABORT`, we can output `key_field->type()` which will be helpful for 
debugging purpose, but we cannot do it with `UNREACHABLE`. So I would still 
prefer `ABORT`.


> On Feb. 9, 2018, 4:26 a.m., Chun-Hung Hsiao wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Lines 1113-1114 (patched)
> > 
> >
> > Since stout is positioned as a standalone library, we should return an 
> > error indicating that this is not supported instead of arbitrarily 
> > terminate the program.

I think the reason we do not return error is this code should never be called, 
otherwise that means something really bad happens which is not a normal error 
that we should return. Actually in stout, `ABORT` and `UNREACHABLE` are widely 
used when an unrecoverable issue occurs.


- Qian


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


On Dec. 21, 2017, 10:07 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59987/
> ---
> 
> (Updated Dec. 21, 2017, 10:07 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Chun-Hung Hsiao, and Zhitao Li.
> 
> 
> Bugs: MESOS-7656
> https://issues.apache.org/jira/browse/MESOS-7656
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Map is a feature of proto2 syntax, but it can only be compiled
> with proto3 compiler, see the following discussion for details:
> https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4
> We have already upgraded the compiler from 2.6.1 to 3.3.0 in
> MESOS-7228, however, to use protobuf map in Mesos code, we also
> need to add the protobuf map support to the code in Mesos for
> converting protobuf message to JSON object and parsing JSON
> object as protobuf message, that is what I have done in this patch.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/protobuf.hpp 
> baad12648dd78ab72ea4277f4c7f99da16696a40 
> 
> 
> Diff: https://reviews.apache.org/r/59987/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 59987: Added protobuf map support.

[Chun-Hung Hsiao]

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



Overall good. Thanks for your efforts!


3rdparty/stout/include/stout/protobuf.hpp
Lines 499-500 (patched)


How about using `UNREACHABLE` instead?



3rdparty/stout/include/stout/protobuf.hpp
Lines 502-503 (patched)


Deprecated or not, it cannot be the type of a key, so let's merge this case 
with the above cases.



3rdparty/stout/include/stout/protobuf.hpp
Lines 1045-1046 (patched)


Ditto.



3rdparty/stout/include/stout/protobuf.hpp
Lines 1045-1051 (patched)


Ditto.



3rdparty/stout/include/stout/protobuf.hpp
Lines 1113-1114 (patched)


Since stout is positioned as a standalone library, we should return an 
error indicating that this is not supported instead of arbitrarily terminate 
the program.



3rdparty/stout/include/stout/protobuf.hpp
Lines 942-943 (original), 1180-1181 (patched)


Not yours, but returning an error seems better for the same argument above.



3rdparty/stout/include/stout/protobuf.hpp
Lines 1005-1006 (original), 1243-1244 (patched)


Ditto.


- Chun-Hung Hsiao


On Dec. 21, 2017, 2:07 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59987/
> ---
> 
> (Updated Dec. 21, 2017, 2:07 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Chun-Hung Hsiao, and Zhitao Li.
> 
> 
> Bugs: MESOS-7656
> https://issues.apache.org/jira/browse/MESOS-7656
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Map is a feature of proto2 syntax, but it can only be compiled
> with proto3 compiler, see the following discussion for details:
> https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4
> We have already upgraded the compiler from 2.6.1 to 3.3.0 in
> MESOS-7228, however, to use protobuf map in Mesos code, we also
> need to add the protobuf map support to the code in Mesos for
> converting protobuf message to JSON object and parsing JSON
> object as protobuf message, that is what I have done in this patch.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/protobuf.hpp 
> baad12648dd78ab72ea4277f4c7f99da16696a40 
> 
> 
> Diff: https://reviews.apache.org/r/59987/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 59987: Added protobuf map support.

[Chun-Hung Hsiao]

> On Sept. 22, 2017, 11:14 p.m., Chun-Hung Hsiao wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Line 607 (original), 640 (patched)
> > 
> >
> > How about a partial specialization for `google::protobuf::Map`?
> 
> Qian Zhang wrote:
> Why do we need to do that? What do we want to achieve with such a partial 
> specialization?
> 
> Chun-Hung Hsiao wrote:
> For providing the `parse>(const JSON::Value& 
> value)` function. Say if we want to create a protobuf message `m` that 
> contains a `map map_field`, we cannot just do 
> `m.mutable_map_field()->CopyFrom(parse(json))`.
>  Instead we probably need something like 
> `m.mutable_map_field()->swap(parse 
> Chun-Hung Hsiao wrote:
> Oh we probably cannot just apply `swap` on an r-value. But you know want 
> I mean ;)
> 
> Qian Zhang wrote:
> So you mean just like the partial specialization for 
> `google::protobuf::RepeatedPtrField`:
> ```
> template 
> struct Parse>
> ```
> We can add a partial specialization for `google::protobuf::Map`, right? I 
> see there are a lot of places in our code to call the partial specialization 
> for `google::protobuf::RepeatedPtrField`, but I am not sure if we have a need 
> to call the partial specialization for `google::protobuf::Map` in our code.

Consider stout can be standalone it would be nice to have it for completeness. 
But let's do it later when needed. Dropping this issue for now.


- Chun-Hung


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


On Dec. 21, 2017, 2:07 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59987/
> ---
> 
> (Updated Dec. 21, 2017, 2:07 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Chun-Hung Hsiao, and Zhitao Li.
> 
> 
> Bugs: MESOS-7656
> https://issues.apache.org/jira/browse/MESOS-7656
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Map is a feature of proto2 syntax, but it can only be compiled
> with proto3 compiler, see the following discussion for details:
> https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4
> We have already upgraded the compiler from 2.6.1 to 3.3.0 in
> MESOS-7228, however, to use protobuf map in Mesos code, we also
> need to add the protobuf map support to the code in Mesos for
> converting protobuf message to JSON object and parsing JSON
> object as protobuf message, that is what I have done in this patch.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/protobuf.hpp 
> baad12648dd78ab72ea4277f4c7f99da16696a40 
> 
> 
> Diff: https://reviews.apache.org/r/59987/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 59987: Added protobuf map support.

[Qian Zhang]

> On Oct. 17, 2017, 7:15 p.m., Benjamin Bannier wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Line 1 (original), 1 (patched)
> > 
> >
> > We should call out that we have a hard requirement on proto3 now in the 
> > `CHANGELOG`.
> > 
> > I believe while we currently bundle protobuf-3.3.0 we do not depend on 
> > any proto3 features, and users might still use an unbundled protobuf-2.6.1 
> > or similar (e.g., distro packages). With this change such a setup needs to 
> > be updated.
> 
> Qian Zhang wrote:
> Good point! I am not sure how many users are using unbundled 
> protobuf-2.6.1 or similar. Do you see it will be a problem if we ask them to 
> update to proto3? Maybe we should ask in user & dev list before merging this 
> patch.

Chun has mentioned it in the CHANGLOG in this patch: 
https://reviews.apache.org/r/64823/ , and he also made protobuf version 3+ a 
hard requirement for building Mesos in this patch: 
https://reviews.apache.org/r/64822/ .


- Qian


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


On Dec. 21, 2017, 10:07 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59987/
> ---
> 
> (Updated Dec. 21, 2017, 10:07 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Chun-Hung Hsiao, and Zhitao Li.
> 
> 
> Bugs: MESOS-7656
> https://issues.apache.org/jira/browse/MESOS-7656
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Map is a feature of proto2 syntax, but it can only be compiled
> with proto3 compiler, see the following discussion for details:
> https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4
> We have already upgraded the compiler from 2.6.1 to 3.3.0 in
> MESOS-7228, however, to use protobuf map in Mesos code, we also
> need to add the protobuf map support to the code in Mesos for
> converting protobuf message to JSON object and parsing JSON
> object as protobuf message, that is what I have done in this patch.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/protobuf.hpp 
> baad12648dd78ab72ea4277f4c7f99da16696a40 
> 
> 
> Diff: https://reviews.apache.org/r/59987/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 59987: Added protobuf map support.

[Qian Zhang]

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

(Updated Dec. 21, 2017, 10:07 a.m.)


Review request for mesos, Anand Mazumdar, Chun-Hung Hsiao, and Zhitao Li.


Changes
---

Fixed a comment and code errors.


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


Repository: mesos


Description
---

Map is a feature of proto2 syntax, but it can only be compiled
with proto3 compiler, see the following discussion for details:
https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4
We have already upgraded the compiler from 2.6.1 to 3.3.0 in
MESOS-7228, however, to use protobuf map in Mesos code, we also
need to add the protobuf map support to the code in Mesos for
converting protobuf message to JSON object and parsing JSON
object as protobuf message, that is what I have done in this patch.


Diffs (updated)
-

  3rdparty/stout/include/stout/protobuf.hpp 
baad12648dd78ab72ea4277f4c7f99da16696a40 


Diff: https://reviews.apache.org/r/59987/diff/4/

Changes: https://reviews.apache.org/r/59987/diff/3-4/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 59987: Added protobuf map support.

[Qian Zhang]

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

(Updated Dec. 19, 2017, 9:50 p.m.)


Review request for mesos, Anand Mazumdar and Zhitao Li.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

Map is a feature of proto2 syntax, but it can only be compiled
with proto3 compiler, see the following discussion for details:
https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4
We have already upgraded the compiler from 2.6.1 to 3.3.0 in
MESOS-7228, however, to use protobuf map in Mesos code, we also
need to add the protobuf map support to the code in Mesos for
converting protobuf message to JSON object and parsing JSON
object as protobuf message, that is what I have done in this patch.


Diffs (updated)
-

  3rdparty/stout/include/stout/protobuf.hpp 
baad12648dd78ab72ea4277f4c7f99da16696a40 


Diff: https://reviews.apache.org/r/59987/diff/3/

Changes: https://reviews.apache.org/r/59987/diff/2-3/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 59987: Added protobuf map support.

[Qian Zhang]

> On Sept. 23, 2017, 7:14 a.m., Chun-Hung Hsiao wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Line 607 (original), 640 (patched)
> > 
> >
> > How about a partial specialization for `google::protobuf::Map`?
> 
> Qian Zhang wrote:
> Why do we need to do that? What do we want to achieve with such a partial 
> specialization?
> 
> Chun-Hung Hsiao wrote:
> For providing the `parse>(const JSON::Value& 
> value)` function. Say if we want to create a protobuf message `m` that 
> contains a `map map_field`, we cannot just do 
> `m.mutable_map_field()->CopyFrom(parse(json))`.
>  Instead we probably need something like 
> `m.mutable_map_field()->swap(parse 
> Chun-Hung Hsiao wrote:
> Oh we probably cannot just apply `swap` on an r-value. But you know want 
> I mean ;)

So you mean just like the partial specialization for 
`google::protobuf::RepeatedPtrField`:
```
template 
struct Parse>
```
We can add a partial specialization for `google::protobuf::Map`, right? I see 
there are a lot of places in our code to call the partial specialization for 
`google::protobuf::RepeatedPtrField`, but I am not sure if we have a need to 
call the partial specialization for `google::protobuf::Map` in our code.


- Qian


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


On July 4, 2017, 10:43 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59987/
> ---
> 
> (Updated July 4, 2017, 10:43 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Zhitao Li.
> 
> 
> Bugs: MESOS-7656
> https://issues.apache.org/jira/browse/MESOS-7656
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Map is a feature of proto2 syntax, but it can only be compiled
> with proto3 compiler, see the following discussion for details:
> https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4
> We have already upgraded the compiler from 2.6.1 to 3.3.0 in
> MESOS-7228, however, to use protobuf map in Mesos code, we also
> need to add the protobuf map support to the code in Mesos for
> converting protobuf message to JSON object and parsing JSON
> object as protobuf message, that is what I have done in this patch.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/protobuf.hpp 
> 15690b66cc4ae0c1bf2c2176d73c385ca75d3c20 
> 
> 
> Diff: https://reviews.apache.org/r/59987/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 59987: Added protobuf map support.

[Qian Zhang]

> On Oct. 17, 2017, 7:15 p.m., Benjamin Bannier wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Line 1 (original), 1 (patched)
> > 
> >
> > We should call out that we have a hard requirement on proto3 now in the 
> > `CHANGELOG`.
> > 
> > I believe while we currently bundle protobuf-3.3.0 we do not depend on 
> > any proto3 features, and users might still use an unbundled protobuf-2.6.1 
> > or similar (e.g., distro packages). With this change such a setup needs to 
> > be updated.

Good point! I am not sure how many users are using unbundled protobuf-2.6.1 or 
similar. Do you see it will be a problem if we ask them to update to proto3? 
Maybe we should ask in user & dev list before merging this patch.


- Qian


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


On July 4, 2017, 10:43 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59987/
> ---
> 
> (Updated July 4, 2017, 10:43 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Zhitao Li.
> 
> 
> Bugs: MESOS-7656
> https://issues.apache.org/jira/browse/MESOS-7656
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Map is a feature of proto2 syntax, but it can only be compiled
> with proto3 compiler, see the following discussion for details:
> https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4
> We have already upgraded the compiler from 2.6.1 to 3.3.0 in
> MESOS-7228, however, to use protobuf map in Mesos code, we also
> need to add the protobuf map support to the code in Mesos for
> converting protobuf message to JSON object and parsing JSON
> object as protobuf message, that is what I have done in this patch.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/protobuf.hpp 
> 15690b66cc4ae0c1bf2c2176d73c385ca75d3c20 
> 
> 
> Diff: https://reviews.apache.org/r/59987/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 59987: Added protobuf map support.

[Benjamin Bannier]

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




3rdparty/stout/include/stout/protobuf.hpp
Line 1 (original), 1 (patched)


We should call out that we have a hard requirement on proto3 now in the 
`CHANGELOG`.

I believe while we currently bundle protobuf-3.3.0 we do not depend on any 
proto3 features, and users might still use an unbundled protobuf-2.6.1 or 
similar (e.g., distro packages). With this change such a setup needs to be 
updated.


- Benjamin Bannier


On July 4, 2017, 4:43 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59987/
> ---
> 
> (Updated July 4, 2017, 4:43 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Zhitao Li.
> 
> 
> Bugs: MESOS-7656
> https://issues.apache.org/jira/browse/MESOS-7656
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Map is a feature of proto2 syntax, but it can only be compiled
> with proto3 compiler, see the following discussion for details:
> https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4
> We have already upgraded the compiler from 2.6.1 to 3.3.0 in
> MESOS-7228, however, to use protobuf map in Mesos code, we also
> need to add the protobuf map support to the code in Mesos for
> converting protobuf message to JSON object and parsing JSON
> object as protobuf message, that is what I have done in this patch.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/protobuf.hpp 
> 15690b66cc4ae0c1bf2c2176d73c385ca75d3c20 
> 
> 
> Diff: https://reviews.apache.org/r/59987/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 59987: Added protobuf map support.

[Chun-Hung Hsiao]

> On Sept. 22, 2017, 11:19 p.m., Chun-Hung Hsiao wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Lines 433 (patched)
> > 
> >
> > The key can also be of an integral type.
> 
> Qian Zhang wrote:
> Yeah, I had the same concern before. But in [our 
> code](https://github.com/apache/mesos/blob/1.4.0/3rdparty/stout/include/stout/json.hpp#L190),
>  we define `JSON::Object::values` as a `string` to `JSON::Value` map, so I am 
> not sure how we can support non-string key.

I think that's because JSON cannos use a numeric value as a key. But we can do 
things like the following in JSON:
```
{
  "1": 1.0,
  "2": 2.0
}
```
Given that ve already know the key type of the map from the reflection, we 
could parse the string into the appropriate integral key type.


- Chun-Hung


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


On July 4, 2017, 2:43 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59987/
> ---
> 
> (Updated July 4, 2017, 2:43 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Zhitao Li.
> 
> 
> Bugs: MESOS-7656
> https://issues.apache.org/jira/browse/MESOS-7656
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Map is a feature of proto2 syntax, but it can only be compiled
> with proto3 compiler, see the following discussion for details:
> https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4
> We have already upgraded the compiler from 2.6.1 to 3.3.0 in
> MESOS-7228, however, to use protobuf map in Mesos code, we also
> need to add the protobuf map support to the code in Mesos for
> converting protobuf message to JSON object and parsing JSON
> object as protobuf message, that is what I have done in this patch.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/protobuf.hpp 
> 15690b66cc4ae0c1bf2c2176d73c385ca75d3c20 
> 
> 
> Diff: https://reviews.apache.org/r/59987/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 59987: Added protobuf map support.

[Chun-Hung Hsiao]

> On Sept. 22, 2017, 11:14 p.m., Chun-Hung Hsiao wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Lines 413 (patched)
> > 
> >
> > Not sure if it is cleaner to parse the map here  because we're 
> > duplicating the `apply_visitor` call here.
> > 
> > Another possible implementation is to move this into `parse()` with an 
> > additional `bool mapField = false` parameter. In `parse()`, if `mapFiled` 
> > is set to true, we can set up the key and replace `message` by a `Message` 
> > pointer to the value before passing it into `apply_visitor`.
> > 
> > Thoughts?
> 
> Qian Zhang wrote:
> I think calling `apply_visitor` here should not be a problem. Actually in 
> the code 
> [here](https://github.com/apache/mesos/blob/1.4.0/3rdparty/stout/include/stout/protobuf.hpp#L538),
>  we already call `apply_visitor` to handle array, so I think it should be OK 
> for us call `apply_visitor` here to handle map.

Yeah it's fine to call `apply_visitor` here. I was just thinking if the code is 
more maintainable if we put the parsing logic together. But I don't know the 
answer here so I didn't open an issue. I'll just leave this to you and Anand ;)


> On Sept. 22, 2017, 11:14 p.m., Chun-Hung Hsiao wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Lines 421 (patched)
> > 
> >
> > Although it seems that `*entry` would not be managed by `repeated_ref` 
> > after `repeated_ref.Add(*entry)`, I cannot find such a guarantee from 
> > protobuf's documentation. How about doing `entry = 
> > reflection->AddMessage(message, field)` here instead, so that we don't need 
> > `repeated_ref`, and the memory management is consistent with the other 
> > cases?
> 
> Qian Zhang wrote:
> I am not sure how we can do `entry = reflection->AddMessage(message, 
> field)` here, do you mean calling `AddMessage()` to add a map message? Then 
> what we should do afterward? I think we still need to parse each entry in the 
> map one by one.
> 
> Actually, using `GetMutableRepeatedFieldRef()` to handle the map field is 
> a way that a Google guy told me, please see the following mail thread for 
> details:
> https://groups.google.com/d/msg/protobuf/nNZ_ItflbLE/x7hLZ1GtAAAJ

The wire format of a map is the same as a repeated field, so we should be able 
to use either `GetMutableRepatedFieldRef.Add(...)` or `AddMessage` to add a map 
entry. Once you get an entry you can just do what you have done in Line 
424--440. It seems to me that we cloud avoid an addition 
construction/destruction of the entry protobuf object with this way.


> On Sept. 22, 2017, 11:14 p.m., Chun-Hung Hsiao wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Line 607 (original), 640 (patched)
> > 
> >
> > How about a partial specialization for `google::protobuf::Map`?
> 
> Qian Zhang wrote:
> Why do we need to do that? What do we want to achieve with such a partial 
> specialization?

For providing the `parse>(const JSON::Value& 
value)` function. Say if we want to create a protobuf message `m` that contains 
a `map map_field`, we cannot just do 
`m.mutable_map_field()->CopyFrom(parse(json))`.
 Instead we probably need something like 
`m.mutable_map_field()->swap(parse On Sept. 22, 2017, 11:14 p.m., Chun-Hung Hsiao wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Line 868 (original), 901 (patched)
> > 
> >
> > How about iterating through `reflection->GetRepeatedMessage()` here 
> > instead?
> 
> Qian Zhang wrote:
> Can you elaborate a bit?

Since a proto2 map is the same as a repearted field in its wire format, we can 
do
```
for (int i = 0; i < fieldSize; ++i) {
  const google::protobuf::Message& entry = 
reflection->GetRepeatedMessage(message, field, i);
  const google::protobuf::Reflection* entryReflection = entry.GetReflection();
  ...
}
```


- Chun-Hung


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


On July 4, 2017, 2:43 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59987/
> ---
> 
> (Updated July 4, 2017, 2:43 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Zhitao Li.
> 
> 
> Bugs: MESOS-7656
> https://issues.apache.org/jira/browse/MESOS-7656
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Map is a feature of proto2 syntax, but it can only be compiled
> with proto3 compiler, see the following

Re: Review Request 59987: Added protobuf map support.

[Qian Zhang]

> On Sept. 23, 2017, 7:14 a.m., Chun-Hung Hsiao wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Lines 413 (patched)
> > 
> >
> > Not sure if it is cleaner to parse the map here  because we're 
> > duplicating the `apply_visitor` call here.
> > 
> > Another possible implementation is to move this into `parse()` with an 
> > additional `bool mapField = false` parameter. In `parse()`, if `mapFiled` 
> > is set to true, we can set up the key and replace `message` by a `Message` 
> > pointer to the value before passing it into `apply_visitor`.
> > 
> > Thoughts?

I think calling `apply_visitor` here should not be a problem. Actually in the 
code 
[here](https://github.com/apache/mesos/blob/1.4.0/3rdparty/stout/include/stout/protobuf.hpp#L538),
 we already call `apply_visitor` to handle array, so I think it should be OK 
for us call `apply_visitor` here to handle map.


> On Sept. 23, 2017, 7:14 a.m., Chun-Hung Hsiao wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Lines 421 (patched)
> > 
> >
> > Although it seems that `*entry` would not be managed by `repeated_ref` 
> > after `repeated_ref.Add(*entry)`, I cannot find such a guarantee from 
> > protobuf's documentation. How about doing `entry = 
> > reflection->AddMessage(message, field)` here instead, so that we don't need 
> > `repeated_ref`, and the memory management is consistent with the other 
> > cases?

I am not sure how we can do `entry = reflection->AddMessage(message, field)` 
here, do you mean calling `AddMessage()` to add a map message? Then what we 
should do afterward? I think we still need to parse each entry in the map one 
by one.

Actually, using `GetMutableRepeatedFieldRef()` to handle the map field is a way 
that a Google guy told me, please see the following mail thread for details:
https://groups.google.com/d/msg/protobuf/nNZ_ItflbLE/x7hLZ1GtAAAJ


> On Sept. 23, 2017, 7:14 a.m., Chun-Hung Hsiao wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Line 607 (original), 640 (patched)
> > 
> >
> > How about a partial specialization for `google::protobuf::Map`?

Why do we need to do that? What do we want to achieve with such a partial 
specialization?


> On Sept. 23, 2017, 7:14 a.m., Chun-Hung Hsiao wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Line 868 (original), 901 (patched)
> > 
> >
> > How about iterating through `reflection->GetRepeatedMessage()` here 
> > instead?

Can you elaborate a bit?


- Qian


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


On July 4, 2017, 10:43 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59987/
> ---
> 
> (Updated July 4, 2017, 10:43 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Zhitao Li.
> 
> 
> Bugs: MESOS-7656
> https://issues.apache.org/jira/browse/MESOS-7656
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Map is a feature of proto2 syntax, but it can only be compiled
> with proto3 compiler, see the following discussion for details:
> https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4
> We have already upgraded the compiler from 2.6.1 to 3.3.0 in
> MESOS-7228, however, to use protobuf map in Mesos code, we also
> need to add the protobuf map support to the code in Mesos for
> converting protobuf message to JSON object and parsing JSON
> object as protobuf message, that is what I have done in this patch.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/protobuf.hpp 
> 15690b66cc4ae0c1bf2c2176d73c385ca75d3c20 
> 
> 
> Diff: https://reviews.apache.org/r/59987/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 59987: Added protobuf map support.

[Qian Zhang]

> On Sept. 23, 2017, 7:19 a.m., Chun-Hung Hsiao wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Lines 433 (patched)
> > 
> >
> > The key can also be of an integral type.

Yeah, I had the same concern before. But in [our 
code](https://github.com/apache/mesos/blob/1.4.0/3rdparty/stout/include/stout/json.hpp#L190),
 we define `JSON::Object::values` as a `string` to `JSON::Value` map, so I am 
not sure how we can support non-string key.


> On Sept. 23, 2017, 7:19 a.m., Chun-Hung Hsiao wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Lines 925 (patched)
> > 
> >
> > The key can also be of an integral type. Same below.

Ditto.


- Qian


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


On July 4, 2017, 10:43 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59987/
> ---
> 
> (Updated July 4, 2017, 10:43 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Zhitao Li.
> 
> 
> Bugs: MESOS-7656
> https://issues.apache.org/jira/browse/MESOS-7656
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Map is a feature of proto2 syntax, but it can only be compiled
> with proto3 compiler, see the following discussion for details:
> https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4
> We have already upgraded the compiler from 2.6.1 to 3.3.0 in
> MESOS-7228, however, to use protobuf map in Mesos code, we also
> need to add the protobuf map support to the code in Mesos for
> converting protobuf message to JSON object and parsing JSON
> object as protobuf message, that is what I have done in this patch.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/protobuf.hpp 
> 15690b66cc4ae0c1bf2c2176d73c385ca75d3c20 
> 
> 
> Diff: https://reviews.apache.org/r/59987/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 59987: Added protobuf map support.

[Chun-Hung Hsiao]

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




3rdparty/stout/include/stout/protobuf.hpp
Lines 433 (patched)


The key can also be of an integral type.



3rdparty/stout/include/stout/protobuf.hpp
Lines 925 (patched)


The key can also be of an integral type. Same below.


- Chun-Hung Hsiao


On July 4, 2017, 2:43 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59987/
> ---
> 
> (Updated July 4, 2017, 2:43 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Zhitao Li.
> 
> 
> Bugs: MESOS-7656
> https://issues.apache.org/jira/browse/MESOS-7656
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Map is a feature of proto2 syntax, but it can only be compiled
> with proto3 compiler, see the following discussion for details:
> https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4
> We have already upgraded the compiler from 2.6.1 to 3.3.0 in
> MESOS-7228, however, to use protobuf map in Mesos code, we also
> need to add the protobuf map support to the code in Mesos for
> converting protobuf message to JSON object and parsing JSON
> object as protobuf message, that is what I have done in this patch.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/protobuf.hpp 
> 15690b66cc4ae0c1bf2c2176d73c385ca75d3c20 
> 
> 
> Diff: https://reviews.apache.org/r/59987/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 59987: Added protobuf map support.

[Chun-Hung Hsiao]

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




3rdparty/stout/include/stout/protobuf.hpp
Lines 413 (patched)


Not sure if it is cleaner to parse the map here  because we're duplicating 
the `apply_visitor` call here.

Another possible implementation is to move this into `parse()` with an 
additional `bool mapField = false` parameter. In `parse()`, if `mapFiled` is 
set to true, we can set up the key and replace `message` by a `Message` pointer 
to the value before passing it into `apply_visitor`.

Thoughts?



3rdparty/stout/include/stout/protobuf.hpp
Lines 421 (patched)


Although it seems that `*entry` would not be managed by `repeated_ref` 
after `repeated_ref.Add(*entry)`, I cannot find such a guarantee from 
protobuf's documentation. How about doing `entry = 
reflection->AddMessage(message, field)` here instead, so that we don't need 
`repeated_ref`, and the memory management is consistent with the other cases?



3rdparty/stout/include/stout/protobuf.hpp
Line 607 (original), 640 (patched)


How about a partial specialization for `google::protobuf::Map`?



3rdparty/stout/include/stout/protobuf.hpp
Line 868 (original), 901 (patched)


How about iterating through `reflection->GetRepeatedMessage()` here instead?


- Chun-Hung Hsiao


On July 4, 2017, 2:43 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59987/
> ---
> 
> (Updated July 4, 2017, 2:43 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Zhitao Li.
> 
> 
> Bugs: MESOS-7656
> https://issues.apache.org/jira/browse/MESOS-7656
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Map is a feature of proto2 syntax, but it can only be compiled
> with proto3 compiler, see the following discussion for details:
> https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4
> We have already upgraded the compiler from 2.6.1 to 3.3.0 in
> MESOS-7228, however, to use protobuf map in Mesos code, we also
> need to add the protobuf map support to the code in Mesos for
> converting protobuf message to JSON object and parsing JSON
> object as protobuf message, that is what I have done in this patch.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/protobuf.hpp 
> 15690b66cc4ae0c1bf2c2176d73c385ca75d3c20 
> 
> 
> Diff: https://reviews.apache.org/r/59987/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 59987: Added protobuf map support.

[Qian Zhang]

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

(Updated July 4, 2017, 10:43 a.m.)


Review request for mesos, Anand Mazumdar and Zhitao Li.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Map is a feature of proto2 syntax, but it can only be compiled
with proto3 compiler, see the following discussion for details:
https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4
We have already upgraded the compiler from 2.6.1 to 3.3.0 in
MESOS-7228, however, to use protobuf map in Mesos code, we also
need to add the protobuf map support to the code in Mesos for
converting protobuf message to JSON object and parsing JSON
object as protobuf message, that is what I have done in this patch.


Diffs (updated)
-

  3rdparty/stout/include/stout/protobuf.hpp 
15690b66cc4ae0c1bf2c2176d73c385ca75d3c20 


Diff: https://reviews.apache.org/r/59987/diff/2/

Changes: https://reviews.apache.org/r/59987/diff/1-2/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 59987: Added protobuf map support.

[Qian Zhang]

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

(Updated June 18, 2017, 8:42 p.m.)


Review request for mesos, Anand Mazumdar and Zhitao Li.


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


Repository: mesos


Description
---

Map is a feature of proto2 syntax, but it can only be compiled
with proto3 compiler, see the following discussion for details:
https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4
We have already upgraded the compiler from 2.6.1 to 3.3.0 in
MESOS-7228, however, to use protobuf map in Mesos code, we also
need to add the protobuf map support to the code in Mesos for
converting protobuf message to JSON object and parsing JSON
object as protobuf message, that is what I have done in this patch.


Diffs
-

  3rdparty/stout/include/stout/protobuf.hpp 
2735354256f35d2792c3690bcc8fc61c3f6d9524 


Diff: https://reviews.apache.org/r/59987/diff/1/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 59987: Added protobuf map support.

[Qian Zhang]

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

(Updated June 18, 2017, 8:39 p.m.)


Review request for mesos, Anand Mazumdar and Zhitao Li.


Repository: mesos


Description
---

Map is a feature of proto2 syntax, but it can only be compiled
with proto3 compiler, see the following discussion for details:
https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4
We have already upgraded the compiler from 2.6.1 to 3.3.0 in
MESOS-7228, however, to use protobuf map in Mesos code, we also
need to add the protobuf map support to the code in Mesos for
converting protobuf message to JSON object and parsing JSON
object as protobuf message, that is what I have done in this patch.


Diffs
-

  3rdparty/stout/include/stout/protobuf.hpp 
2735354256f35d2792c3690bcc8fc61c3f6d9524 


Diff: https://reviews.apache.org/r/59987/diff/1/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 59987: Added protobuf map support.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [59987]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On June 12, 2017, 10:01 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59987/
> ---
> 
> (Updated June 12, 2017, 10:01 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Zhitao Li.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Map is a feature of proto2 syntax, but it can only be compiled
> with proto3 compiler, see the following discussion for details:
> https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4
> We have already upgraded the compiler from 2.6.1 to 3.3.0 in
> MESOS-7228, however, to use protobuf map in Mesos code, we also
> need to add the protobuf map support to the code in Mesos for
> converting protobuf message to JSON object and parsing JSON
> object as protobuf message, that is what I have done in this patch.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/protobuf.hpp 
> 2735354256f35d2792c3690bcc8fc61c3f6d9524 
> 
> 
> Diff: https://reviews.apache.org/r/59987/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Review Request 59987: Added protobuf map support.

[Qian Zhang]

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

Review request for mesos, Anand Mazumdar and Zhitao Li.


Repository: mesos


Description
---

Map is a feature of proto2 syntax, but it can only be compiled
with proto3 compiler, see the following discussion for details:
https://groups.google.com/forum/#!topic/protobuf/p4WxcplrlA4
We have already upgraded the compiler from 2.6.1 to 3.3.0 in
MESOS-7228, however, to use protobuf map in Mesos code, we also
need to add the protobuf map support to the code in Mesos for
converting protobuf message to JSON object and parsing JSON
object as protobuf message, that is what I have done in this patch.


Diffs
-

  3rdparty/stout/include/stout/protobuf.hpp 
2735354256f35d2792c3690bcc8fc61c3f6d9524 


Diff: https://reviews.apache.org/r/59987/diff/1/


Testing
---


Thanks,

Qian Zhang