----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71158/#review216914 -----------------------------------------------------------
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. 3rdparty/stout/tests/protobuf_tests.cpp Lines 730-731 (patched) <https://reviews.apache.org/r/71158/#comment304154> 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? - Benjamin Mahler On July 26, 2019, 12:16 a.m., Meng Zhu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71158/ > ----------------------------------------------------------- > > (Updated July 26, 2019, 12:16 a.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 added jsonify support for such mapping. > > Also revised a test to test the jsonify map support. > > > Diffs > ----- > > 3rdparty/stout/include/stout/protobuf.hpp > 4b3db7eb807723359af85e8a0324b176e49a954a > 3rdparty/stout/tests/protobuf_tests.cpp > 95cdc67cdab0aeef2ce18aa0c99bc2952c2b5dc5 > > > Diff: https://reviews.apache.org/r/71158/diff/2/ > > > Testing > ------- > > make check > > > Thanks, > > Meng Zhu > >
