mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chun-Hung Hsiao <chhs...@mesosphere.io>
Subject Re: Review Request 59987: Added protobuf map support to stout JSON<->protobuf conversion.
Date Thu, 08 Mar 2018 02:46:57 GMT


> On March 1, 2018, 4:10 a.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/?
> 
> 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<int64_t> key = numify<int64_t>(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)
> > <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
> 
> 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<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);
>                     ...
>                   }
>                   ...
>     ```

Re: `JSON::Value`: It seems that there's no need to declare a new `key` variable, and just
pass `JSON::Value(name)` into `apply_visitor`.

Re: `JSON::String` <-> bools and ints: Yes. Ben's actually suggesting that we should
allow this conversion in general, an an enhancement of stout's JSON support, so these conversions
need not to be specific for map support. For example, if we implement this enhancement, we
could parse JSON outputed from Google's JSON function. You could probably prepend another
patch for this enhancement.


> On March 1, 2018, 4:10 a.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?
> 
> Qian Zhang wrote:
>     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.

The `entry` itself doesn't depend on `name` and `value`, so let's hoist them outside the loop.


> On March 1, 2018, 4:10 a.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!
> 
> Qian Zhang wrote:
>     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?

I agree on having another ticket. We probably need to raise this in the dev mailing list since
this could be a breaking change.


> On March 1, 2018, 4:10 a.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.
> 
> Qian Zhang wrote:
>     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 :-(

Let's do it for maps and objects. It's unfortunate but seems we have to keep the array case
as is.


- Chun-Hung


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


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


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