Re: Review Request 39611: Remove binary `data` fields from state endpoints.

2015-11-04 Thread Ben Mahler

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

Ship it!


Thanks Joseph, just a minor comment about the upgrade note.


docs/upgrades.md (line 16)


Can we be clear about exactly what is being removed here and give some 
background (link to the ticket)? Specifically, it would be helpful to say 
exactly which fields instead of referring to arbitrary binary data.


- Ben Mahler


On Nov. 3, 2015, 1:06 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39611/
> ---
> 
> (Updated Nov. 3, 2015, 1:06 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Artem Harutyunyan.
> 
> 
> Bugs: MESOS-3771
> https://issues.apache.org/jira/browse/MESOS-3771
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We do not encode or otherwise preprocess the raw binary in the ExecutorInfo 
> or TaskInfo `data` fields before outputting them to JSON via the state 
> endpoints.  This means that the outputted JSON may be invalid (i.e. non-UTF8) 
> if `data` was filled in with arbitrary bytes.
> 
> The `data` fields are being removed because the state endpoints should not be 
> writing binary data in the first place.
> 
> 
> Diffs
> -
> 
>   docs/upgrades.md 3854e59f68d53892b2673a4542ea2aaf96936cbb 
>   src/common/http.cpp f56d8a178e1f61d91adfeaad32b47718adbc4379 
>   src/slave/http.cpp d6df97fb64077fb051dd78fe35bf04e8be0331ff 
> 
> Diff: https://reviews.apache.org/r/39611/diff/
> 
> 
> Testing
> ---
> 
> `make check` and visually checked markdown for correctness.
> 
> Apparently, none of our tests check these fields.  
> 
> (Discovered via manual tests) Our JSON parser (PicoJson) is not restrictive 
> enough to catch the invalid-JSON that results.  It will happily parse the 
> binary blobs (roundtripping successfully).
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>



Re: Review Request 39611: Remove binary `data` fields from state endpoints.

2015-11-04 Thread Joseph Wu

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

(Updated Nov. 4, 2015, 4:02 p.m.)


Review request for mesos, Ben Mahler and Artem Harutyunyan.


Changes
---

Expand on the upgrades.md comment.


Bugs: MESOS-3771
https://issues.apache.org/jira/browse/MESOS-3771


Repository: mesos


Description
---

We do not encode or otherwise preprocess the raw binary in the ExecutorInfo or 
TaskInfo `data` fields before outputting them to JSON via the state endpoints.  
This means that the outputted JSON may be invalid (i.e. non-UTF8) if `data` was 
filled in with arbitrary bytes.

The `data` fields are being removed because the state endpoints should not be 
writing binary data in the first place.


Diffs (updated)
-

  docs/upgrades.md 3854e59f68d53892b2673a4542ea2aaf96936cbb 
  src/common/http.cpp f56d8a178e1f61d91adfeaad32b47718adbc4379 
  src/slave/http.cpp d6df97fb64077fb051dd78fe35bf04e8be0331ff 

Diff: https://reviews.apache.org/r/39611/diff/


Testing
---

`make check` and visually checked markdown for correctness.

Apparently, none of our tests check these fields.  

(Discovered via manual tests) Our JSON parser (PicoJson) is not restrictive 
enough to catch the invalid-JSON that results.  It will happily parse the 
binary blobs (roundtripping successfully).


Thanks,

Joseph Wu



Re: Review Request 39611: Remove binary `data` fields from state endpoints.

2015-11-04 Thread Guangya Liu

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

Ship it!


Ship It!

- Guangya Liu


On 十一月 5, 2015, 12:02 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39611/
> ---
> 
> (Updated 十一月 5, 2015, 12:02 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Artem Harutyunyan.
> 
> 
> Bugs: MESOS-3771
> https://issues.apache.org/jira/browse/MESOS-3771
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We do not encode or otherwise preprocess the raw binary in the ExecutorInfo 
> or TaskInfo `data` fields before outputting them to JSON via the state 
> endpoints.  This means that the outputted JSON may be invalid (i.e. non-UTF8) 
> if `data` was filled in with arbitrary bytes.
> 
> The `data` fields are being removed because the state endpoints should not be 
> writing binary data in the first place.
> 
> 
> Diffs
> -
> 
>   docs/upgrades.md 3854e59f68d53892b2673a4542ea2aaf96936cbb 
>   src/common/http.cpp f56d8a178e1f61d91adfeaad32b47718adbc4379 
>   src/slave/http.cpp d6df97fb64077fb051dd78fe35bf04e8be0331ff 
> 
> Diff: https://reviews.apache.org/r/39611/diff/
> 
> 
> Testing
> ---
> 
> `make check` and visually checked markdown for correctness.
> 
> Apparently, none of our tests check these fields.  
> 
> (Discovered via manual tests) Our JSON parser (PicoJson) is not restrictive 
> enough to catch the invalid-JSON that results.  It will happily parse the 
> binary blobs (roundtripping successfully).
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>



Re: Review Request 39611: Remove binary `data` fields from state endpoints.

2015-11-04 Thread Joseph Wu

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

(Updated Nov. 4, 2015, 4:11 p.m.)


Review request for mesos, Ben Mahler and Artem Harutyunyan.


Changes
---

Rebased on the other change to upgrades.md.


Bugs: MESOS-3771
https://issues.apache.org/jira/browse/MESOS-3771


Repository: mesos


Description
---

We do not encode or otherwise preprocess the raw binary in the ExecutorInfo or 
TaskInfo `data` fields before outputting them to JSON via the state endpoints.  
This means that the outputted JSON may be invalid (i.e. non-UTF8) if `data` was 
filled in with arbitrary bytes.

The `data` fields are being removed because the state endpoints should not be 
writing binary data in the first place.


Diffs (updated)
-

  docs/upgrades.md c7bf6a226fb6f5af8ef42bb9e9b843099fdd4e71 
  src/common/http.cpp abb4c6bc2fe547bdc3a15682bfa680a52acc75a8 
  src/slave/http.cpp d6df97fb64077fb051dd78fe35bf04e8be0331ff 

Diff: https://reviews.apache.org/r/39611/diff/


Testing
---

`make check` and visually checked markdown for correctness.

Apparently, none of our tests check these fields.  

(Discovered via manual tests) Our JSON parser (PicoJson) is not restrictive 
enough to catch the invalid-JSON that results.  It will happily parse the 
binary blobs (roundtripping successfully).


Thanks,

Joseph Wu



Re: Review Request 39611: Remove binary `data` fields from state endpoints.

2015-11-02 Thread Kapil Arya


> On Nov. 2, 2015, 3:52 p.m., Neil Conway wrote:
> > How about backward compatibility? Adding a note to docs/upgrades.md seems a 
> > good idea, at the very least. Are we pretty confident that no one else is 
> > looking at this data, and/or we're happy to break anyone that is?

Good point about backward compatibility. However, if the `data` field contains 
random bytes that might result into the entire json becoming "unrenderable", 
then it's broken anyways. Also note that we don't expose `TaskInfo::data` and 
`TaskStatus::data` fields over state.json either. We can use the dev/user 
mailing lists to get a feel for it.


- Kapil


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


On Nov. 2, 2015, 3:21 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39611/
> ---
> 
> (Updated Nov. 2, 2015, 3:21 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Artem Harutyunyan.
> 
> 
> Bugs: MESOS-3771
> https://issues.apache.org/jira/browse/MESOS-3771
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We do not encode or otherwise preprocess the raw binary in the ExecutorInfo 
> or TaskInfo `data` fields before outputting them to JSON via the state 
> endpoints.  This means that the outputted JSON may be invalid (i.e. non-UTF8) 
> if `data` was filled in with arbitrary bytes.
> 
> The `data` fields are being removed because the state endpoints should not be 
> writing binary data in the first place.
> 
> 
> Diffs
> -
> 
>   src/common/http.cpp f56d8a178e1f61d91adfeaad32b47718adbc4379 
>   src/slave/http.cpp d6df97fb64077fb051dd78fe35bf04e8be0331ff 
> 
> Diff: https://reviews.apache.org/r/39611/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Apparently, none of our tests check these fields.  
> 
> (Discovered via manual tests) Our JSON parser (PicoJson) is not restrictive 
> enough to catch the invalid-JSON that results.  It will happily parse the 
> binary blobs (roundtripping successfully).
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>



Re: Review Request 39611: Remove binary `data` fields from state endpoints.

2015-11-02 Thread Neil Conway

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


How about backward compatibility? Adding a note to docs/upgrades.md seems a 
good idea, at the very least. Are we pretty confident that no one else is 
looking at this data, and/or we're happy to break anyone that is?

- Neil Conway


On Nov. 2, 2015, 8:21 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39611/
> ---
> 
> (Updated Nov. 2, 2015, 8:21 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Artem Harutyunyan.
> 
> 
> Bugs: MESOS-3771
> https://issues.apache.org/jira/browse/MESOS-3771
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We do not encode or otherwise preprocess the raw binary in the ExecutorInfo 
> or TaskInfo `data` fields before outputting them to JSON via the state 
> endpoints.  This means that the outputted JSON may be invalid (i.e. non-UTF8) 
> if `data` was filled in with arbitrary bytes.
> 
> The `data` fields are being removed because the state endpoints should not be 
> writing binary data in the first place.
> 
> 
> Diffs
> -
> 
>   src/common/http.cpp f56d8a178e1f61d91adfeaad32b47718adbc4379 
>   src/slave/http.cpp d6df97fb64077fb051dd78fe35bf04e8be0331ff 
> 
> Diff: https://reviews.apache.org/r/39611/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Apparently, none of our tests check these fields.  
> 
> (Discovered via manual tests) Our JSON parser (PicoJson) is not restrictive 
> enough to catch the invalid-JSON that results.  It will happily parse the 
> binary blobs (roundtripping successfully).
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>



Re: Review Request 39611: Remove binary `data` fields from state endpoints.

2015-11-02 Thread Joseph Wu


> On Nov. 2, 2015, 12:52 p.m., Neil Conway wrote:
> > How about backward compatibility? Adding a note to docs/upgrades.md seems a 
> > good idea, at the very least. Are we pretty confident that no one else is 
> > looking at this data, and/or we're happy to break anyone that is?
> 
> Kapil Arya wrote:
> Good point about backward compatibility. However, if the `data` field 
> contains random bytes that might result into the entire json becoming 
> "unrenderable", then it's broken anyways. Also note that we don't expose 
> `TaskInfo::data` and `TaskStatus::data` fields over state.json either. We can 
> use the dev/user mailing lists to get a feel for it.

Yeah, I emailed the dev list a while ago about this: 
http://www.mail-archive.com/dev@mesos.apache.org/msg33536.html

Note: The `TaskInfo::data` fields are part of the agent's state endpoint.


- Joseph


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


On Nov. 2, 2015, 12:21 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39611/
> ---
> 
> (Updated Nov. 2, 2015, 12:21 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Artem Harutyunyan.
> 
> 
> Bugs: MESOS-3771
> https://issues.apache.org/jira/browse/MESOS-3771
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We do not encode or otherwise preprocess the raw binary in the ExecutorInfo 
> or TaskInfo `data` fields before outputting them to JSON via the state 
> endpoints.  This means that the outputted JSON may be invalid (i.e. non-UTF8) 
> if `data` was filled in with arbitrary bytes.
> 
> The `data` fields are being removed because the state endpoints should not be 
> writing binary data in the first place.
> 
> 
> Diffs
> -
> 
>   src/common/http.cpp f56d8a178e1f61d91adfeaad32b47718adbc4379 
>   src/slave/http.cpp d6df97fb64077fb051dd78fe35bf04e8be0331ff 
> 
> Diff: https://reviews.apache.org/r/39611/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Apparently, none of our tests check these fields.  
> 
> (Discovered via manual tests) Our JSON parser (PicoJson) is not restrictive 
> enough to catch the invalid-JSON that results.  It will happily parse the 
> binary blobs (roundtripping successfully).
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>



Re: Review Request 39611: Remove binary `data` fields from state endpoints.

2015-11-02 Thread Guangya Liu

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

Ship it!


I think that the upgrade documentshould also be updated if this field is 
removed.

- Guangya Liu


On 十一月 2, 2015, 8:21 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39611/
> ---
> 
> (Updated 十一月 2, 2015, 8:21 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Artem Harutyunyan.
> 
> 
> Bugs: MESOS-3771
> https://issues.apache.org/jira/browse/MESOS-3771
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We do not encode or otherwise preprocess the raw binary in the ExecutorInfo 
> or TaskInfo `data` fields before outputting them to JSON via the state 
> endpoints.  This means that the outputted JSON may be invalid (i.e. non-UTF8) 
> if `data` was filled in with arbitrary bytes.
> 
> The `data` fields are being removed because the state endpoints should not be 
> writing binary data in the first place.
> 
> 
> Diffs
> -
> 
>   src/common/http.cpp f56d8a178e1f61d91adfeaad32b47718adbc4379 
>   src/slave/http.cpp d6df97fb64077fb051dd78fe35bf04e8be0331ff 
> 
> Diff: https://reviews.apache.org/r/39611/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Apparently, none of our tests check these fields.  
> 
> (Discovered via manual tests) Our JSON parser (PicoJson) is not restrictive 
> enough to catch the invalid-JSON that results.  It will happily parse the 
> binary blobs (roundtripping successfully).
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>



Re: Review Request 39611: Remove binary `data` fields from state endpoints.

2015-11-02 Thread Mesos ReviewBot

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


Patch looks great!

Reviews applied: [39611]

All tests passed.

- Mesos ReviewBot


On Nov. 2, 2015, 8:21 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39611/
> ---
> 
> (Updated Nov. 2, 2015, 8:21 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Artem Harutyunyan.
> 
> 
> Bugs: MESOS-3771
> https://issues.apache.org/jira/browse/MESOS-3771
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We do not encode or otherwise preprocess the raw binary in the ExecutorInfo 
> or TaskInfo `data` fields before outputting them to JSON via the state 
> endpoints.  This means that the outputted JSON may be invalid (i.e. non-UTF8) 
> if `data` was filled in with arbitrary bytes.
> 
> The `data` fields are being removed because the state endpoints should not be 
> writing binary data in the first place.
> 
> 
> Diffs
> -
> 
>   src/common/http.cpp f56d8a178e1f61d91adfeaad32b47718adbc4379 
>   src/slave/http.cpp d6df97fb64077fb051dd78fe35bf04e8be0331ff 
> 
> Diff: https://reviews.apache.org/r/39611/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Apparently, none of our tests check these fields.  
> 
> (Discovered via manual tests) Our JSON parser (PicoJson) is not restrictive 
> enough to catch the invalid-JSON that results.  It will happily parse the 
> binary blobs (roundtripping successfully).
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>



Re: Review Request 39611: Remove binary `data` fields from state endpoints.

2015-11-02 Thread Joseph Wu

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

(Updated Nov. 2, 2015, 5:06 p.m.)


Review request for mesos, Ben Mahler and Artem Harutyunyan.


Changes
---

Add a note to `upgrades.md`.


Bugs: MESOS-3771
https://issues.apache.org/jira/browse/MESOS-3771


Repository: mesos


Description
---

We do not encode or otherwise preprocess the raw binary in the ExecutorInfo or 
TaskInfo `data` fields before outputting them to JSON via the state endpoints.  
This means that the outputted JSON may be invalid (i.e. non-UTF8) if `data` was 
filled in with arbitrary bytes.

The `data` fields are being removed because the state endpoints should not be 
writing binary data in the first place.


Diffs (updated)
-

  docs/upgrades.md 3854e59f68d53892b2673a4542ea2aaf96936cbb 
  src/common/http.cpp f56d8a178e1f61d91adfeaad32b47718adbc4379 
  src/slave/http.cpp d6df97fb64077fb051dd78fe35bf04e8be0331ff 

Diff: https://reviews.apache.org/r/39611/diff/


Testing (updated)
---

`make check` and visually checked markdown for correctness.

Apparently, none of our tests check these fields.  

(Discovered via manual tests) Our JSON parser (PicoJson) is not restrictive 
enough to catch the invalid-JSON that results.  It will happily parse the 
binary blobs (roundtripping successfully).


Thanks,

Joseph Wu



Re: Review Request 39611: Remove binary `data` fields from state endpoints.

2015-11-02 Thread Guangya Liu

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



docs/upgrades.md (line 16)


Shall we highlight `data` here?


- Guangya Liu


On 十一月 3, 2015, 1:06 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39611/
> ---
> 
> (Updated 十一月 3, 2015, 1:06 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Artem Harutyunyan.
> 
> 
> Bugs: MESOS-3771
> https://issues.apache.org/jira/browse/MESOS-3771
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We do not encode or otherwise preprocess the raw binary in the ExecutorInfo 
> or TaskInfo `data` fields before outputting them to JSON via the state 
> endpoints.  This means that the outputted JSON may be invalid (i.e. non-UTF8) 
> if `data` was filled in with arbitrary bytes.
> 
> The `data` fields are being removed because the state endpoints should not be 
> writing binary data in the first place.
> 
> 
> Diffs
> -
> 
>   docs/upgrades.md 3854e59f68d53892b2673a4542ea2aaf96936cbb 
>   src/common/http.cpp f56d8a178e1f61d91adfeaad32b47718adbc4379 
>   src/slave/http.cpp d6df97fb64077fb051dd78fe35bf04e8be0331ff 
> 
> Diff: https://reviews.apache.org/r/39611/diff/
> 
> 
> Testing
> ---
> 
> `make check` and visually checked markdown for correctness.
> 
> Apparently, none of our tests check these fields.  
> 
> (Discovered via manual tests) Our JSON parser (PicoJson) is not restrictive 
> enough to catch the invalid-JSON that results.  It will happily parse the 
> binary blobs (roundtripping successfully).
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>



Re: Review Request 39611: Remove binary `data` fields from state endpoints.

2015-11-02 Thread Mesos ReviewBot

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


Patch looks great!

Reviews applied: [39611]

All tests passed.

- Mesos ReviewBot


On Nov. 3, 2015, 1:06 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39611/
> ---
> 
> (Updated Nov. 3, 2015, 1:06 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Artem Harutyunyan.
> 
> 
> Bugs: MESOS-3771
> https://issues.apache.org/jira/browse/MESOS-3771
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We do not encode or otherwise preprocess the raw binary in the ExecutorInfo 
> or TaskInfo `data` fields before outputting them to JSON via the state 
> endpoints.  This means that the outputted JSON may be invalid (i.e. non-UTF8) 
> if `data` was filled in with arbitrary bytes.
> 
> The `data` fields are being removed because the state endpoints should not be 
> writing binary data in the first place.
> 
> 
> Diffs
> -
> 
>   docs/upgrades.md 3854e59f68d53892b2673a4542ea2aaf96936cbb 
>   src/common/http.cpp f56d8a178e1f61d91adfeaad32b47718adbc4379 
>   src/slave/http.cpp d6df97fb64077fb051dd78fe35bf04e8be0331ff 
> 
> Diff: https://reviews.apache.org/r/39611/diff/
> 
> 
> Testing
> ---
> 
> `make check` and visually checked markdown for correctness.
> 
> Apparently, none of our tests check these fields.  
> 
> (Discovered via manual tests) Our JSON parser (PicoJson) is not restrictive 
> enough to catch the invalid-JSON that results.  It will happily parse the 
> binary blobs (roundtripping successfully).
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>