Re: Review Request 54295: Added a synchronous version of loop for io::read/write/redirect.

[Benjamin Mahler]

> On Dec. 6, 2016, 2:12 a.m., Benjamin Mahler wrote:
> > Is it possible to split the optional pid change from the discard logic 
> > change? If so that would be great!
> > 
> > Looks like a chunk from the next patch slipped into this one?

Also, would be great to update the description to say that this also updates 
the discard logic to resolve racing (and which race in particular).


- Benjamin


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


On Dec. 5, 2016, 4:35 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54295/
> ---
> 
> (Updated Dec. 5, 2016, 4:35 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a synchronous version of loop for io::read/write/redirect.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/loop.hpp 
> a78ea7da8a5a778d816a64fb46d9479cc2a4ed70 
>   3rdparty/libprocess/src/http.cpp fc55bda0fae8a43794fb938718b3a720dc342473 
>   3rdparty/libprocess/src/io.cpp e81f279ed4bf92f75ad2427550ca822a9b03cca5 
> 
> Diff: https://reviews.apache.org/r/54295/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 54295: Added a synchronous version of loop for io::read/write/redirect.

[Benjamin Mahler]

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


Fix it, then Ship it!




Is it possible to split the optional pid change from the discard logic change? 
If so that would be great!

Looks like a chunk from the next patch slipped into this one?


3rdparty/libprocess/include/process/loop.hpp (line 102)


Where is ValueType defined? It looks like it's in the next review? Did you 
mean to include this chunk in the next review instead of this one? These don't 
seem to be used in this patch?



3rdparty/libprocess/include/process/loop.hpp (line 291)


Maybe add a comment about what the mutex is for?


- Benjamin Mahler


On Dec. 5, 2016, 4:35 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54295/
> ---
> 
> (Updated Dec. 5, 2016, 4:35 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a synchronous version of loop for io::read/write/redirect.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/loop.hpp 
> a78ea7da8a5a778d816a64fb46d9479cc2a4ed70 
>   3rdparty/libprocess/src/http.cpp fc55bda0fae8a43794fb938718b3a720dc342473 
>   3rdparty/libprocess/src/io.cpp e81f279ed4bf92f75ad2427550ca822a9b03cca5 
> 
> Diff: https://reviews.apache.org/r/54295/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 54295: Added a synchronous version of loop for io::read/write/redirect.

[Benjamin Hindman]

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

(Updated Dec. 5, 2016, 4:35 a.m.)


Review request for mesos, Benjamin Mahler and Jie Yu.


Repository: mesos


Description
---

Added a synchronous version of loop for io::read/write/redirect.


Diffs (updated)
-

  3rdparty/libprocess/include/process/loop.hpp 
a78ea7da8a5a778d816a64fb46d9479cc2a4ed70 
  3rdparty/libprocess/src/http.cpp fc55bda0fae8a43794fb938718b3a720dc342473 
  3rdparty/libprocess/src/io.cpp e81f279ed4bf92f75ad2427550ca822a9b03cca5 

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


Testing
---

make check


Thanks,

Benjamin Hindman

Re: Review Request 54295: Added a synchronous version of loop for io::read/write/redirect.

[Benjamin Mahler]

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


Fix it, then Ship it!





3rdparty/libprocess/include/process/loop.hpp (lines 32 - 36)


Can you update this paragraph accordingly?



3rdparty/libprocess/include/process/loop.hpp (lines 91 - 94)


This needs an update?



3rdparty/libprocess/include/process/loop.hpp (lines 169 - 178)


Do you want to capture a lambda here like you did for `discard` and 
`continuation`?


- Benjamin Mahler


On Dec. 2, 2016, 6:45 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54295/
> ---
> 
> (Updated Dec. 2, 2016, 6:45 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a synchronous version of loop for io::read/write/redirect.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/loop.hpp 
> a78ea7da8a5a778d816a64fb46d9479cc2a4ed70 
>   3rdparty/libprocess/src/io.cpp e81f279ed4bf92f75ad2427550ca822a9b03cca5 
> 
> Diff: https://reviews.apache.org/r/54295/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 54295: Added a synchronous version of loop for io::read/write/redirect.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [54295]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On Dec. 2, 2016, 6:45 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54295/
> ---
> 
> (Updated Dec. 2, 2016, 6:45 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a synchronous version of loop for io::read/write/redirect.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/loop.hpp 
> a78ea7da8a5a778d816a64fb46d9479cc2a4ed70 
>   3rdparty/libprocess/src/io.cpp e81f279ed4bf92f75ad2427550ca822a9b03cca5 
> 
> Diff: https://reviews.apache.org/r/54295/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>