You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Meng Zhu <mz...@mesosphere.io> on 2019/07/29 21:52:54 UTC
Re: Review Request 71158: Fixed non-standard mapping for protobuf map
fields in jsonify.
-----------------------------------------------------------
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.
Changes
-------
Addressed Ben's comment.
Summary (updated)
-----------------
Fixed non-standard mapping for protobuf map fields in jsonify.
Bugs: MESOS-9901
https://issues.apache.org/jira/browse/MESOS-9901
Repository: mesos
Description (updated)
-------
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 (updated)
-----
3rdparty/stout/include/stout/protobuf.hpp 4b3db7eb807723359af85e8a0324b176e49a954a
Diff: https://reviews.apache.org/r/71158/diff/3/
Changes: https://reviews.apache.org/r/71158/diff/2-3/
Testing
-------
make check
Thanks,
Meng Zhu
Re: Review Request 71158: Fixed non-standard mapping for protobuf map
fields in jsonify.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
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
>
>