mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrei Sekretenko <asekrete...@mesosphere.io>
Subject Re: Review Request 71158: Added proper support for protobuf Map in jsonify.
Date Thu, 25 Jul 2019 14:12:09 GMT

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




3rdparty/stout/include/stout/protobuf.hpp
Lines 901 (patched)
<https://reviews.apache.org/r/71158/#comment304104>

    A typo? (`Nmae`)



3rdparty/stout/include/stout/protobuf.hpp
Lines 955-958 (patched)
<https://reviews.apache.org/r/71158/#comment304105>

    Is there any reason not to use `Reflection::GetRepeatedFieldRef()`?


- Andrei Sekretenko


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


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