----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71158/#review216926 -----------------------------------------------------------
Fix it, then Ship it! Looks good to me modulo turning the error logging into hard failures, can you also have Chun take a look at this since he's familiar with this code? 3rdparty/stout/include/stout/protobuf.hpp Lines 970-972 (patched) <https://reviews.apache.org/r/71158/#comment304159> No need to print this message, CHECK_EQ will print the CHECK statement, and the two values for you. E.g. ``` F0729 15:47:53.824182 18896 hierarchical_allocator_tests.cpp:5171] Check failed: foo == 2 (1 vs. 2) ``` 3rdparty/stout/include/stout/protobuf.hpp Lines 1021-1023 (patched) <https://reviews.apache.org/r/71158/#comment304160> This should be a hard failure with CHECK_NE or a LOG(FATAL) instead of LOG(ERROR)? 3rdparty/stout/include/stout/protobuf.hpp Lines 1037-1038 (patched) <https://reviews.apache.org/r/71158/#comment304161> Ditto here, LOG_FATAL? - Benjamin Mahler On July 29, 2019, 9: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, 9: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 > >
