mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Qian Zhang <zhq527...@gmail.com>
Subject Re: Review Request 59987: Added protobuf map support.
Date Tue, 06 Mar 2018 09:25:29 GMT


> 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)
> > <https://reviews.apache.org/r/59987/diff/5/?file=1965083#file1965083line32>
> >
> >     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)
> > <https://reviews.apache.org/r/59987/diff/5/?file=1965083#file1965083line388>
> >
> >     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<int64_t> key = numify<int64_t>(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)
> > <https://reviews.apache.org/r/59987/diff/5/?file=1965083#file1965083line391>
> >
> >     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<Nothing> 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<Nothing> 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<char>'
```
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<Nothing> 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<Nothing> 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<Nothing> 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:
              {
                Try<int64_t> key = numify<int64_t>(name);
                ...
              }
              case google::protobuf::FieldDescriptor::TYPE_UINT64:
              case google::protobuf::FieldDescriptor::TYPE_FIXED64:
              {
                Try<uint64_t> key = numify<uint64_t>(name);
                ...
              }
              ...
```


> On March 1, 2018, 12:10 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Lines 394-398 (patched)
> > <https://reviews.apache.org/r/59987/diff/5/?file=1965083#file1965083line394>
> >
> >     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;
> >     ```

Sure.


> On March 1, 2018, 12:10 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Lines 400-401 (patched)
> > <https://reviews.apache.org/r/59987/diff/5/?file=1965083#file1965083line400>
> >
> >     Can you move this out of the loop?

It needs the variable `entry` which is defined inside of the `foreachpair` loop, so I think
we still need to keep `reflection` inside of the loop.


> On March 1, 2018, 12:10 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Lines 1036-1037 (patched)
> > <https://reviews.apache.org/r/59987/diff/5/?file=1965083#file1965083line1037>
> >
> >     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!

Yeah, in the doc below, I see the 64 bit integers in protobuf message will be converted into
JSON strings.
https://developers.google.com/protocol-buffers/docs/proto3#json

How about I create a separate ticket for it and fix it there?


> On March 1, 2018, 12:10 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Lines 1039-1043 (patched)
> > <https://reviews.apache.org/r/59987/diff/5/?file=1965083#file1965083line1040>
> >
> >     Is there a way to abstract out this logic into a lambda here and re-use it?
E.g.
> >     
> >     ```
> >     template <typename T>
> >     JSON::Value value_for_field(
> >         Message* entry,
> >         google::protobuf::Reflection* reflection,
> >         google::protobuf::FieldDescriptor* field)
> >     {
> >       switch (type) {
> >         case ...:
> >         case TYPE_INT64:
> >           return ...;
> >       }
> >     };
> >     ```
> >     
> >     Then you just do the following here and elsewhere:
> >     
> >     ```
> >     map.values[key] = value_for_field(entry, reflection, field);
> >     ```
> >     
> >     I've glossed over some details, but I hope you get the idea and can figure out
how to make it work across the different cases here.

Do you mean moving the big `"switch ... case"` introduced in my patch for map into a method
`value_for_field()`, and use it to replace the other two big `"switch ... case"` for array
and object (see below) too?
https://github.com/apache/mesos/blob/1.5.0/3rdparty/stout/include/stout/protobuf.hpp#L864:L923
https://github.com/apache/mesos/blob/1.5.0/3rdparty/stout/include/stout/protobuf.hpp#L927:L986

I like the idea and I think it is doable for map and object, but for array, it is a bit different
since we will call different protobuf APIs, e.g., `reflection->GetInt32` (map and object)
v.s. `reflection->GetRepeatedInt32()` (array). So I think it may not be able to cover array
:-(


- Qian


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


On March 1, 2018, 7:27 a.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59987/
> -----------------------------------------------------------
> 
> (Updated March 1, 2018, 7:27 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 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message