You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Qian Zhang <zh...@gmail.com> on 2017/06/12 02:01:12 UTC
Review Request 59987: Added protobuf map support.
-----------------------------------------------------------
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
Re: Review Request 59987: Added protobuf map support.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 59987: Added protobuf map support.
Posted by Qian Zhang <zh...@gmail.com>.
> On Oct. 17, 2017, 7:15 p.m., Benjamin Bannier wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Line 1 (original), 1 (patched)
> > <https://reviews.apache.org/r/59987/diff/2/?file=1768786#file1768786line1>
> >
> > 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.
Posted by Qian Zhang <zh...@gmail.com>.
> On Oct. 17, 2017, 7:15 p.m., Benjamin Bannier wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Line 1 (original), 1 (patched)
> > <https://reviews.apache.org/r/59987/diff/2/?file=1768786#file1768786line1>
> >
> > 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.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/59987/#comment265296>
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.
Posted by Qian Zhang <zh...@gmail.com>.
> On Sept. 23, 2017, 7:14 a.m., Chun-Hung Hsiao wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Line 607 (original), 640 (patched)
> > <https://reviews.apache.org/r/59987/diff/2/?file=1768786#file1768786line640>
> >
> > 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<google::protobuf::Map<K, V>>(const JSON::Value& value)` function. Say if we want to create a protobuf message `m` that contains a `map<string, string> map_field`, we cannot just do `m.mutable_map_field()->CopyFrom(parse<google::protobuf::RepeatedPtrField<...>(json))`. Instead we probably need something like `m.mutable_map_field()->swap(parse<google::protobuf::Map(json))`.
>
> 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 <typename T>
struct Parse<google::protobuf::RepeatedPtrField<T>>
```
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.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
> On Sept. 22, 2017, 11:14 p.m., Chun-Hung Hsiao wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Line 607 (original), 640 (patched)
> > <https://reviews.apache.org/r/59987/diff/2/?file=1768786#file1768786line640>
> >
> > 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<google::protobuf::Map<K, V>>(const JSON::Value& value)` function. Say if we want to create a protobuf message `m` that contains a `map<string, string> map_field`, we cannot just do `m.mutable_map_field()->CopyFrom(parse<google::protobuf::RepeatedPtrField<...>(json))`. Instead we probably need something like `m.mutable_map_field()->swap(parse<google::protobuf::Map(json))`.
>
> 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 <typename T>
> struct Parse<google::protobuf::RepeatedPtrField<T>>
> ```
> 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.
Posted by Qian Zhang <zh...@gmail.com>.
> On Sept. 23, 2017, 7:14 a.m., Chun-Hung Hsiao wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Lines 413 (patched)
> > <https://reviews.apache.org/r/59987/diff/2/?file=1768786#file1768786line413>
> >
> > 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)
> > <https://reviews.apache.org/r/59987/diff/2/?file=1768786#file1768786line421>
> >
> > 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)
> > <https://reviews.apache.org/r/59987/diff/2/?file=1768786#file1768786line640>
> >
> > 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)
> > <https://reviews.apache.org/r/59987/diff/2/?file=1768786#file1768786line901>
> >
> > 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.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
> On Sept. 22, 2017, 11:14 p.m., Chun-Hung Hsiao wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Lines 413 (patched)
> > <https://reviews.apache.org/r/59987/diff/2/?file=1768786#file1768786line413>
> >
> > 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)
> > <https://reviews.apache.org/r/59987/diff/2/?file=1768786#file1768786line421>
> >
> > 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)
> > <https://reviews.apache.org/r/59987/diff/2/?file=1768786#file1768786line640>
> >
> > 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<google::protobuf::Map<K, V>>(const JSON::Value& value)` function. Say if we want to create a protobuf message `m` that contains a `map<string, string> map_field`, we cannot just do `m.mutable_map_field()->CopyFrom(parse<google::protobuf::RepeatedPtrField<...>(json))`. Instead we probably need something like `m.mutable_map_field()->swap(parse<google::protobuf::Map(json))`.
> On Sept. 22, 2017, 11:14 p.m., Chun-Hung Hsiao wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Line 868 (original), 901 (patched)
> > <https://reviews.apache.org/r/59987/diff/2/?file=1768786#file1768786line901>
> >
> > 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 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.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/59987/#comment262403>
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)
<https://reviews.apache.org/r/59987/#comment262404>
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)
<https://reviews.apache.org/r/59987/#comment262406>
How about a partial specialization for `google::protobuf::Map`?
3rdparty/stout/include/stout/protobuf.hpp
Line 868 (original), 901 (patched)
<https://reviews.apache.org/r/59987/#comment262408>
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.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
> On Sept. 22, 2017, 11:19 p.m., Chun-Hung Hsiao wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Lines 433 (patched)
> > <https://reviews.apache.org/r/59987/diff/2/?file=1768786#file1768786line433>
> >
> > 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.
Posted by Qian Zhang <zh...@gmail.com>.
> On Sept. 23, 2017, 7:19 a.m., Chun-Hung Hsiao wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Lines 433 (patched)
> > <https://reviews.apache.org/r/59987/diff/2/?file=1768786#file1768786line433>
> >
> > 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)
> > <https://reviews.apache.org/r/59987/diff/2/?file=1768786#file1768786line925>
> >
> > 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.
Posted by Chun-Hung Hsiao <ch...@mesosphere.io>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/59987/#comment262409>
The key can also be of an integral type.
3rdparty/stout/include/stout/protobuf.hpp
Lines 925 (patched)
<https://reviews.apache.org/r/59987/#comment262410>
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.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
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.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
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.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
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.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
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