> 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 > >
