> On July 29, 2019, 10:42 a.m., Benjamin Mahler wrote: > > Thanks for fixing this!! > > > > - Can you clarify the commit summary / description to say that this is > > "fixing" an issue in handling maps w.r.t to the proto3 standard mapping? > > - Can you follow up on the audit of existing map fields and if there are > > any that get exposed through our API, writing an email to the lists about > > this change? (If there are no fields, mention that here?) > > - It's a more involved change (in terms of looking into it, but will delete > > a lot of our custom code), but it seems worth taking the time now to see if > > we can switch to the built in json mapping logic: (which seems to have > > enough options?) > > https://developers.google.com/protocol-buffers/docs/proto3#json_options. I > > also wonder if it's faster than reflection (see MESOS-9896 and related > > tickets, there is a performance regression in protobuf reflection if we > > upgarde our protobuf library). This would have avoided issues like this one > > in the first place.
- Clarified. - There is no egress using map fields atm. Mentioned this in the description. - Added a TODO, not sure if we should prioritize that now. > On July 29, 2019, 10:42 a.m., Benjamin Mahler wrote: > > 3rdparty/stout/tests/protobuf_tests.cpp > > Lines 730-731 (patched) > > <https://reviews.apache.org/r/71158/diff/2/?file=2157950#file2157950line730> > > > > There seems to be a lot of movement around in the test, and it's hard > > to tell if anything is logically changing does it need to be in this patch? Separated into https://reviews.apache.org/r/71186/ - Meng ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71158/#review216914 ----------------------------------------------------------- On July 29, 2019, 2:52 p.m., Meng Zhu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71158/ > ----------------------------------------------------------- > > (Updated July 29, 2019, 2:52 p.m.) > > > Review request for mesos, Andrei Sekretenko, Benjamin Bannier, and Benjamin > Mahler. > > > Bugs: MESOS-9901 > https://issues.apache.org/jira/browse/MESOS-9901 > > > Repository: mesos > > > Description > ------- > > Before this patch jsonify treats protobuf Map as a regular > repeated field. This means a Map with schema: > > message QuotaConfig { > required string role = 1; > > map<string, Value.Scalar> guarantees = 2; > map<string, Value.Scalar> limits = 3; > } > > may be jsonify to an JSON array: > > { > "configs": [ > { > "role": "role1", > "guarantees": [ > { > "key": "cpus", > "value": { > "value": 1 > } > }, > { > "key": "mem", > "value": { > "value": 512 > } > } > ] > } > ] > } > > Per standard proto3 JSON mapping, the Map type should be mapped > to an JSON object, i.e. > > { > "configs": [ > { > "role": "role1", > "guarantees": { > "cpus": 1, > "mem": 512 > } > } > ] > } > > This patch made jsonify support for such mapping. > > Currently, there is no egress of map fields in the code base, > so this presents no external visible change. > > > Diffs > ----- > > 3rdparty/stout/include/stout/protobuf.hpp > 4b3db7eb807723359af85e8a0324b176e49a954a > > > Diff: https://reviews.apache.org/r/71158/diff/3/ > > > Testing > ------- > > make check > > > Thanks, > > Meng Zhu > >
