You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Chun-Hung Hsiao <ch...@mesosphere.io> on 2017/09/22 23:14:44 UTC
Re: Review Request 59987: Added protobuf map support.
-----------------------------------------------------------
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 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 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 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
>
>