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