Re: Review Request 42030: Added example of a `defer` bug to libprocess README.

[Timothy Chen]

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

Ship it!


Ship It!

- Timothy Chen


On Jan. 12, 2016, 11:34 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42030/
> ---
> 
> (Updated Jan. 12, 2016, 11:34 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Neil Conway, and Timothy Chen.
> 
> 
> Bugs: MESOS-4207
> https://issues.apache.org/jira/browse/MESOS-4207
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added example of a `defer` bug to libprocess README.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/README.md 6a47f6772bb7a74af368ed970af0f1c154a070e3 
> 
> Diff: https://reviews.apache.org/r/42030/diff/
> 
> 
> Testing
> ---
> 
> Viewed the markdown in a github branch: 
> https://github.com/mesosphere/mesos/tree/defer_example/3rdparty/libprocess
> 
> The `defer` documentation is getting pretty verbose at this point :-) I think 
> it can be condensed, and I'd like to do so and incorporate Alex Rojas' input 
> in this follow-up review: https://reviews.apache.org/r/41618/
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 42030: Added example of a `defer` bug to libprocess README.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [42030]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Jan. 12, 2016, 11:34 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42030/
> ---
> 
> (Updated Jan. 12, 2016, 11:34 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Neil Conway, and Timothy Chen.
> 
> 
> Bugs: MESOS-4207
> https://issues.apache.org/jira/browse/MESOS-4207
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added example of a `defer` bug to libprocess README.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/README.md 6a47f6772bb7a74af368ed970af0f1c154a070e3 
> 
> Diff: https://reviews.apache.org/r/42030/diff/
> 
> 
> Testing
> ---
> 
> Viewed the markdown in a github branch: 
> https://github.com/mesosphere/mesos/tree/defer_example/3rdparty/libprocess
> 
> The `defer` documentation is getting pretty verbose at this point :-) I think 
> it can be condensed, and I'd like to do so and incorporate Alex Rojas' input 
> in this follow-up review: https://reviews.apache.org/r/41618/
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 42030: Added example of a `defer` bug to libprocess README.

[Greg Mann]

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

(Updated Jan. 12, 2016, 11:33 p.m.)


Review request for mesos, Ben Mahler, Jojy Varghese, Neil Conway, and Timothy 
Chen.


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


Repository: mesos


Description
---

Added example of a `defer` bug to libprocess README.


Diffs
-

  3rdparty/libprocess/README.md 6a47f6772bb7a74af368ed970af0f1c154a070e3 

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


Testing (updated)
---

Viewed the markdown in a github branch: 
https://github.com/mesosphere/mesos/tree/defer_example/3rdparty/libprocess

The `defer` documentation is getting pretty verbose at this point :-) I think 
it can be condensed, and I'd like to do so and incorporate Alex Rojas' input in 
this follow-up review: https://reviews.apache.org/r/41618/


Thanks,

Greg Mann

Re: Review Request 42030: Added example of a `defer` bug to libprocess README.

[Greg Mann]

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

(Updated Jan. 12, 2016, 11:31 p.m.)


Review request for mesos, Ben Mahler, Jojy Varghese, Neil Conway, and Timothy 
Chen.


Changes
---

Addressed comments, used a simple toy example instead of a Mesos code excerpt.


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


Repository: mesos


Description
---

Added example of a `defer` bug to libprocess README.


Diffs (updated)
-

  3rdparty/libprocess/README.md 6a47f6772bb7a74af368ed970af0f1c154a070e3 

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


Testing
---

Viewed the markdown in a github branch: 
https://github.com/mesosphere/mesos/tree/defer_example/3rdparty/libprocess

Note that in addition to the inclusion of this example, further changes to the 
`defer` documentation preceding the example are currently being considered: 
https://reviews.apache.org/r/41618/


Thanks,

Greg Mann

Re: Review Request 42030: Added example of a `defer` bug to libprocess README.

[Timothy Chen]

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



3rdparty/libprocess/README.md (line 257)


IMO there seems to be too much irrelevant code to make the point, can we 
collapse all the unnecessary code and just keep the ones that can illustrate 
the problem?


- Timothy Chen


On Jan. 8, 2016, 5:01 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42030/
> ---
> 
> (Updated Jan. 8, 2016, 5:01 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Jojy Varghese, Neil Conway, and Timothy 
> Chen.
> 
> 
> Bugs: MESOS-4207
> https://issues.apache.org/jira/browse/MESOS-4207
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added example of a `defer` bug to libprocess README.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/README.md 6a47f6772bb7a74af368ed970af0f1c154a070e3 
> 
> Diff: https://reviews.apache.org/r/42030/diff/
> 
> 
> Testing
> ---
> 
> Viewed the markdown in a github branch: 
> https://github.com/mesosphere/mesos/tree/defer_example/3rdparty/libprocess
> 
> Note that in addition to the inclusion of this example, further changes to 
> the `defer` documentation preceding the example are currently being 
> considered: https://reviews.apache.org/r/41618/
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 42030: Added example of a `defer` bug to libprocess README.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [42030]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Jan. 8, 2016, 5:01 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42030/
> ---
> 
> (Updated Jan. 8, 2016, 5:01 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Jojy Varghese, Neil Conway, and Timothy 
> Chen.
> 
> 
> Bugs: MESOS-4207
> https://issues.apache.org/jira/browse/MESOS-4207
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added example of a `defer` bug to libprocess README.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/README.md 6a47f6772bb7a74af368ed970af0f1c154a070e3 
> 
> Diff: https://reviews.apache.org/r/42030/diff/
> 
> 
> Testing
> ---
> 
> Viewed the markdown in a github branch: 
> https://github.com/mesosphere/mesos/tree/defer_example/3rdparty/libprocess
> 
> Note that in addition to the inclusion of this example, further changes to 
> the `defer` documentation preceding the example are currently being 
> considered: https://reviews.apache.org/r/41618/
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 42030: Added example of a `defer` bug to libprocess README.

[Neil Conway]

> On Jan. 7, 2016, 11:21 p.m., Neil Conway wrote:
> > 3rdparty/libprocess/README.md, line 323
> > 
> >
> > Do we really have to look at all the code paths within `doHttpGet`? By 
> > the time `getBlob()` returns to the caller (i.e., by the time anyone might 
> > satisfy the future in question), `doHttpGet` must have been called and 
> > returned already.
> 
> Greg Mann wrote:
> Correct me if I'm wrong, but the execution context of the callbacks 
> registered on this future will be determined by the code path followed in 
> `doHttpGet`. So if we want to confirm that execution remains within this 
> process' context, we need to walk through those paths and confirm that they 
> stay within the process.

Makes sense. Can we clarify that what we care about is the behavior of 
callbacks on the future registered in `doHttpGet`, not the behavior of 
`doHttpGet` itself?


- Neil


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


On Jan. 8, 2016, 5:01 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42030/
> ---
> 
> (Updated Jan. 8, 2016, 5:01 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Jojy Varghese, Neil Conway, and Timothy 
> Chen.
> 
> 
> Bugs: MESOS-4207
> https://issues.apache.org/jira/browse/MESOS-4207
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added example of a `defer` bug to libprocess README.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/README.md 6a47f6772bb7a74af368ed970af0f1c154a070e3 
> 
> Diff: https://reviews.apache.org/r/42030/diff/
> 
> 
> Testing
> ---
> 
> Viewed the markdown in a github branch: 
> https://github.com/mesosphere/mesos/tree/defer_example/3rdparty/libprocess
> 
> Note that in addition to the inclusion of this example, further changes to 
> the `defer` documentation preceding the example are currently being 
> considered: https://reviews.apache.org/r/41618/
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 42030: Added example of a `defer` bug to libprocess README.

[Greg Mann]

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

(Updated Jan. 8, 2016, 5:01 p.m.)


Review request for mesos, Ben Mahler, Jojy Varghese, Neil Conway, and Timothy 
Chen.


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


Repository: mesos


Description
---

Added example of a `defer` bug to libprocess README.


Diffs (updated)
-

  3rdparty/libprocess/README.md 6a47f6772bb7a74af368ed970af0f1c154a070e3 

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


Testing
---

Viewed the markdown in a github branch: 
https://github.com/mesosphere/mesos/tree/defer_example/3rdparty/libprocess

Note that in addition to the inclusion of this example, further changes to the 
`defer` documentation preceding the example are currently being considered: 
https://reviews.apache.org/r/41618/


Thanks,

Greg Mann

Re: Review Request 42030: Added example of a `defer` bug to libprocess README.

[Greg Mann]

> On Jan. 7, 2016, 11:21 p.m., Neil Conway wrote:
> > 3rdparty/libprocess/README.md, line 323
> > 
> >
> > Do we really have to look at all the code paths within `doHttpGet`? By 
> > the time `getBlob()` returns to the caller (i.e., by the time anyone might 
> > satisfy the future in question), `doHttpGet` must have been called and 
> > returned already.

Correct me if I'm wrong, but the execution context of the callbacks registered 
on this future will be determined by the code path followed in `doHttpGet`. So 
if we want to confirm that execution remains within this process' context, we 
need to walk through those paths and confirm that they stay within the process.


- Greg


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


On Jan. 8, 2016, 4:55 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42030/
> ---
> 
> (Updated Jan. 8, 2016, 4:55 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Jojy Varghese, Neil Conway, and Timothy 
> Chen.
> 
> 
> Bugs: MESOS-4207
> https://issues.apache.org/jira/browse/MESOS-4207
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added example of a `defer` bug to libprocess README.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/README.md 6a47f6772bb7a74af368ed970af0f1c154a070e3 
> 
> Diff: https://reviews.apache.org/r/42030/diff/
> 
> 
> Testing
> ---
> 
> Viewed the markdown in a github branch: 
> https://github.com/mesosphere/mesos/tree/defer_example/3rdparty/libprocess
> 
> Note that in addition to the inclusion of this example, further changes to 
> the `defer` documentation preceding the example are currently being 
> considered: https://reviews.apache.org/r/41618/
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 42030: Added example of a `defer` bug to libprocess README.

[Greg Mann]

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

(Updated Jan. 8, 2016, 4:55 p.m.)


Review request for mesos, Ben Mahler, Jojy Varghese, Neil Conway, and Timothy 
Chen.


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


Repository: mesos


Description
---

Added example of a `defer` bug to libprocess README.


Diffs (updated)
-

  3rdparty/libprocess/README.md 6a47f6772bb7a74af368ed970af0f1c154a070e3 

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


Testing
---

Viewed the markdown in a github branch: 
https://github.com/mesosphere/mesos/tree/defer_example/3rdparty/libprocess

Note that in addition to the inclusion of this example, further changes to the 
`defer` documentation preceding the example are currently being considered: 
https://reviews.apache.org/r/41618/


Thanks,

Greg Mann

Re: Review Request 42030: Added example of a `defer` bug to libprocess README.

[Neil Conway]

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



3rdparty/libprocess/README.md (line 247)


We could simplify the wording a bit, I think: "It is instructive to examine 
... One such example."

How about: "An example of a situation in which omitting `defer` lead to a 
bug can be found in a (historical) version of `RegistryClientProcess::getBlob`:"



3rdparty/libprocess/README.md (line 253)


Rather than including the full commit SHA1 in the text, can't we link to 
the appropriate line number in the appropriate commit on Github?



3rdparty/libprocess/README.md (line 323)


Do we really have to look at all the code paths within `doHttpGet`? By the 
time `getBlob()` returns to the caller (i.e., by the time anyone might satisfy 
the future in question), `doHttpGet` must have been called and returned already.


- Neil Conway


On Jan. 7, 2016, 7:46 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42030/
> ---
> 
> (Updated Jan. 7, 2016, 7:46 p.m.)
> 
> 
> Review request for mesos, Jojy Varghese, Neil Conway, and Timothy Chen.
> 
> 
> Bugs: MESOS-4207
> https://issues.apache.org/jira/browse/MESOS-4207
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added example of a `defer` bug to libprocess README.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/README.md 6a47f6772bb7a74af368ed970af0f1c154a070e3 
> 
> Diff: https://reviews.apache.org/r/42030/diff/
> 
> 
> Testing
> ---
> 
> Viewed the markdown in a github branch: 
> https://github.com/mesosphere/mesos/tree/defer_example/3rdparty/libprocess
> 
> Note that in addition to the inclusion of this example, further changes to 
> the `defer` documentation preceding the example are currently being 
> considered: https://reviews.apache.org/r/41618/
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 42030: Added example of a `defer` bug to libprocess README.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [42030]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Jan. 7, 2016, 7:46 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42030/
> ---
> 
> (Updated Jan. 7, 2016, 7:46 p.m.)
> 
> 
> Review request for mesos, Jojy Varghese, Neil Conway, and Timothy Chen.
> 
> 
> Bugs: MESOS-4207
> https://issues.apache.org/jira/browse/MESOS-4207
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added example of a `defer` bug to libprocess README.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/README.md 6a47f6772bb7a74af368ed970af0f1c154a070e3 
> 
> Diff: https://reviews.apache.org/r/42030/diff/
> 
> 
> Testing
> ---
> 
> Viewed the markdown in a github branch: 
> https://github.com/mesosphere/mesos/tree/defer_example/3rdparty/libprocess
> 
> Note that in addition to the inclusion of this example, further changes to 
> the `defer` documentation preceding the example are currently being 
> considered: https://reviews.apache.org/r/41618/
> 
> 
> Thanks,
> 
> Greg Mann
> 
>