mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Meng Zhu <m...@mesosphere.io>
Subject Re: Review Request 71158: Added proper support for protobuf Map in jsonify.
Date Fri, 26 Jul 2019 00:15:58 GMT


> On July 25, 2019, 7:12 a.m., Andrei Sekretenko wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Lines 901 (patched)
> > <https://reviews.apache.org/r/71158/diff/1/?file=2157633#file2157633line901>
> >
> >     A typo? (`Nmae`)

Oops! Fixed.


> On July 25, 2019, 7:12 a.m., Andrei Sekretenko wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Lines 955-958 (patched)
> > <https://reviews.apache.org/r/71158/diff/1/?file=2157633#file2157633line955>
> >
> >     Is there any reason not to use `Reflection::GetRepeatedFieldRef()`?

Thanks, does look cleaner.


- Meng


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71158/#review216857
-----------------------------------------------------------


On July 24, 2019, 5:18 p.m., Meng Zhu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71158/
> -----------------------------------------------------------
> 
> (Updated July 24, 2019, 5:18 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 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/1/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message