Re: Review Request 71158: Added proper support for protobuf Map in jsonify.

2019-07-29 Thread Benjamin Mahler 

---
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)


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 guarantees = 2;
>   map 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.

2019-07-25 Thread Meng Zhu 

---
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 guarantees = 2;
  map 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.

2019-07-25 Thread Meng Zhu 


> On July 25, 2019, 7:12 a.m., Andrei Sekretenko wrote:
> > 3rdparty/stout/include/stout/protobuf.hpp
> > Lines 901 (patched)
> > 
> >
> > 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)
> > 
> >
> > 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 guarantees = 2;
>   map 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.

2019-07-25 Thread Andrei Sekretenko 

---
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)


A typo? (`Nmae`)



3rdparty/stout/include/stout/protobuf.hpp
Lines 955-958 (patched)


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 guarantees = 2;
>   map 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
> 
>

Review Request 71158: Added proper support for protobuf Map in jsonify.

2019-07-24 Thread Meng Zhu 

---
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 guarantees = 2;
  map 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