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/25 00:18:11 UTC
Review Request 71158: Added proper support for protobuf Map in
jsonify.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71158/
-----------------------------------------------------------
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
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
>
>
Re: Review Request 71158: Fixed non-standard mapping for protobuf map
fields in jsonify.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
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 Meng Zhu <mz...@mesosphere.io>.
> On July 29, 2019, 10:42 a.m., Benjamin Mahler wrote:
> > 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.
- Clarified.
- There is no egress using map fields atm. Mentioned this in the description.
- Added a TODO, not sure if we should prioritize that now.
> On July 29, 2019, 10:42 a.m., Benjamin Mahler wrote:
> > 3rdparty/stout/tests/protobuf_tests.cpp
> > Lines 730-731 (patched)
> > <https://reviews.apache.org/r/71158/diff/2/?file=2157950#file2157950line730>
> >
> > 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?
Separated into https://reviews.apache.org/r/71186/
- Meng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71158/#review216914
-----------------------------------------------------------
On July 29, 2019, 2: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, 2: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
>
>
Re: Review Request 71158: Added proper support for protobuf Map 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/#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
>
>
Re: Review Request 71158: Added proper support for protobuf Map in
jsonify.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71158/
-----------------------------------------------------------
(Updated July 25, 2019, 5:16 p.m.)
Review request for mesos, Andrei Sekretenko, Benjamin Bannier, and Benjamin Mahler.
Changes
-------
Addressed Andrei's comments.
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 (updated)
-----
3rdparty/stout/include/stout/protobuf.hpp 4b3db7eb807723359af85e8a0324b176e49a954a
3rdparty/stout/tests/protobuf_tests.cpp 95cdc67cdab0aeef2ce18aa0c99bc2952c2b5dc5
Diff: https://reviews.apache.org/r/71158/diff/2/
Changes: https://reviews.apache.org/r/71158/diff/1-2/
Testing
-------
make check
Thanks,
Meng Zhu
Re: Review Request 71158: Added proper support for protobuf Map in
jsonify.
Posted by Meng Zhu <mz...@mesosphere.io>.
> 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
>
>
Re: Review Request 71158: Added proper support for protobuf Map in
jsonify.
Posted by Andrei Sekretenko <as...@mesosphere.io>.
-----------------------------------------------------------
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
>
>