----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59987/#review198429 -----------------------------------------------------------
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. ``` 3rdparty/stout/include/stout/protobuf.hpp Lines 32 (patched) <https://reviews.apache.org/r/59987/#comment278571> 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 3rdparty/stout/include/stout/protobuf.hpp Lines 388 (patched) <https://reviews.apache.org/r/59987/#comment278572> s/name/key/? 3rdparty/stout/include/stout/protobuf.hpp Lines 391-487 (patched) <https://reviews.apache.org/r/59987/#comment278577> 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 3rdparty/stout/include/stout/protobuf.hpp Lines 394-398 (patched) <https://reviews.apache.org/r/59987/#comment278573> 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; ``` 3rdparty/stout/include/stout/protobuf.hpp Lines 400-401 (patched) <https://reviews.apache.org/r/59987/#comment278578> Can you move this out of the loop? 3rdparty/stout/include/stout/protobuf.hpp Lines 1036-1037 (patched) <https://reviews.apache.org/r/59987/#comment278579> 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! 3rdparty/stout/include/stout/protobuf.hpp Lines 1039-1043 (patched) <https://reviews.apache.org/r/59987/#comment278580> 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. - Benjamin Mahler On Feb. 28, 2018, 11:27 p.m., Qian Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59987/ > ----------------------------------------------------------- > > (Updated Feb. 28, 2018, 11:27 p.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 > >
