You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Greg Mann <gr...@mesosphere.io> on 2015/12/17 00:46:06 UTC
Review Request 41474: Added documentation for `defer` in libprocess
README.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41474/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman, Ben Mahler, Joris Van Remoortere, and Neil Conway.
Bugs: MESOS-3996
https://issues.apache.org/jira/browse/MESOS-3996
Repository: mesos
Description
-------
Added documentation for `defer` in libprocess README.
Diffs
-----
3rdparty/libprocess/README.md c3f309a7dc1c94882c4cc97eeaf0736c2fca0ba5
Diff: https://reviews.apache.org/r/41474/diff/
Testing
-------
Viewed the markdown in a branch on GitHub: https://github.com/mesosphere/mesos/tree/defer_docs/3rdparty/libprocess
Thanks,
Greg Mann
Re: Review Request 41474: Added documentation for `defer` in
libprocess README.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Dec. 18, 2015, 10:43 p.m., Neil Conway wrote:
> > 3rdparty/libprocess/README.md, line 176
> > <https://reviews.apache.org/r/41474/diff/1/?file=1167576#file1167576line176>
> >
> > We should be consistent (between code samples) about whether we're assuming any `using` statements are implied, or restating them at the top of each code sample.
I added `using` statements to the first code sample to make things consistent.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41474/#review111297
-----------------------------------------------------------
On Dec. 19, 2015, 1:26 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41474/
> -----------------------------------------------------------
>
> (Updated Dec. 19, 2015, 1:26 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Joris Van Remoortere, and Neil Conway.
>
>
> Bugs: MESOS-3996
> https://issues.apache.org/jira/browse/MESOS-3996
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added documentation for `defer` in libprocess README.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/README.md c3f309a7dc1c94882c4cc97eeaf0736c2fca0ba5
>
> Diff: https://reviews.apache.org/r/41474/diff/
>
>
> Testing
> -------
>
> Viewed the markdown in a branch on GitHub: https://github.com/mesosphere/mesos/tree/defer_docs/3rdparty/libprocess
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 41474: Added documentation for `defer` in
libprocess README.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41474/#review111297
-----------------------------------------------------------
3rdparty/libprocess/README.md (line 176)
<https://reviews.apache.org/r/41474/#comment171409>
We should be consistent (between code samples) about whether we're assuming any `using` statements are implied, or restating them at the top of each code sample.
- Neil Conway
On Dec. 16, 2015, 11:46 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41474/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2015, 11:46 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Joris Van Remoortere, and Neil Conway.
>
>
> Bugs: MESOS-3996
> https://issues.apache.org/jira/browse/MESOS-3996
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added documentation for `defer` in libprocess README.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/README.md c3f309a7dc1c94882c4cc97eeaf0736c2fca0ba5
>
> Diff: https://reviews.apache.org/r/41474/diff/
>
>
> Testing
> -------
>
> Viewed the markdown in a branch on GitHub: https://github.com/mesosphere/mesos/tree/defer_docs/3rdparty/libprocess
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 41474: Added documentation for `defer` in
libprocess README.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Dec. 18, 2015, 10:37 p.m., Neil Conway wrote:
> > 3rdparty/libprocess/README.md, line 150
> > <https://reviews.apache.org/r/41474/diff/1/?file=1167576#file1167576line150>
> >
> > The need to use `lambda::_1` here is a bit unfortunate.
I added a comment to clarify this.
> On Dec. 18, 2015, 10:37 p.m., Neil Conway wrote:
> > 3rdparty/libprocess/README.md, line 111
> > <https://reviews.apache.org/r/41474/diff/1/?file=1167576#file1167576line111>
> >
> > "with it" or "on it"? (I'm not actually sure.)
I couldn't find a definitive answer on this, but it seems that "on" may be used a bit more commonly?
On Dec. 18, 2015, 10:37 p.m., Greg Mann wrote:
> > There have been non-deterministic bugs that have arisen from missing `defer`s -- i.e., whether a lack of `defer` causes problems can depend on the vagaries of the libprocess scheduler. e.g., https://reviews.apache.org/r/40610/diff/1#index_header -- would it be feasible/useful to walk through a simple example?
Good idea, I've created a JIRA to track this improvement here: https://issues.apache.org/jira/browse/MESOS-4207
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41474/#review111287
-----------------------------------------------------------
On Dec. 19, 2015, 1:26 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41474/
> -----------------------------------------------------------
>
> (Updated Dec. 19, 2015, 1:26 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Joris Van Remoortere, and Neil Conway.
>
>
> Bugs: MESOS-3996
> https://issues.apache.org/jira/browse/MESOS-3996
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added documentation for `defer` in libprocess README.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/README.md c3f309a7dc1c94882c4cc97eeaf0736c2fca0ba5
>
> Diff: https://reviews.apache.org/r/41474/diff/
>
>
> Testing
> -------
>
> Viewed the markdown in a branch on GitHub: https://github.com/mesosphere/mesos/tree/defer_docs/3rdparty/libprocess
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 41474: Added documentation for `defer` in
libprocess README.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41474/#review111287
-----------------------------------------------------------
3rdparty/libprocess/README.md (line 106)
<https://reviews.apache.org/r/41474/#comment171398>
Per discussion, adjust this text to give more context:
* all callbacks are invoked synchronously in the process that satisfies the future
* sometimes this is bad
* you can use defer() to arrange for an operation to be performed asynchronously in the context of a specified process
Then fix up comments in code sample and discussion below.
3rdparty/libprocess/README.md (line 109)
<https://reviews.apache.org/r/41474/#comment171382>
What is "the specified process"?
3rdparty/libprocess/README.md (line 111)
<https://reviews.apache.org/r/41474/#comment171385>
"with it" or "on it"? (I'm not actually sure.)
3rdparty/libprocess/README.md (line 122)
<https://reviews.apache.org/r/41474/#comment171389>
"be invoked"
3rdparty/libprocess/README.md (line 130)
<https://reviews.apache.org/r/41474/#comment171390>
"be invoked"
3rdparty/libprocess/README.md (line 133)
<https://reviews.apache.org/r/41474/#comment171388>
Avoid using "runs" twice in the same sentence.
3rdparty/libprocess/README.md (line 150)
<https://reviews.apache.org/r/41474/#comment171405>
The need to use `lambda::_1` here is a bit unfortunate.
3rdparty/libprocess/README.md (line 156)
<https://reviews.apache.org/r/41474/#comment171399>
"the one in which it is registered" or "the process that satisfies the future"?
3rdparty/libprocess/README.md (line 157)
<https://reviews.apache.org/r/41474/#comment171400>
`self()` is not used in the code sample.
3rdparty/libprocess/README.md (line 158)
<https://reviews.apache.org/r/41474/#comment171401>
There are two callbacks in the code sample above, not clear which one you're referring to.
3rdparty/libprocess/README.md (line 224)
<https://reviews.apache.org/r/41474/#comment171406>
I don't think we should be encouraging people to write code when they are "in doubt" :)
There have been non-deterministic bugs that have arisen from missing `defer`s -- i.e., whether a lack of `defer` causes problems can depend on the vagaries of the libprocess scheduler. e.g., https://reviews.apache.org/r/40610/diff/1#index_header -- would it be feasible/useful to walk through a simple example?
- Neil Conway
On Dec. 16, 2015, 11:46 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41474/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2015, 11:46 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Joris Van Remoortere, and Neil Conway.
>
>
> Bugs: MESOS-3996
> https://issues.apache.org/jira/browse/MESOS-3996
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added documentation for `defer` in libprocess README.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/README.md c3f309a7dc1c94882c4cc97eeaf0736c2fca0ba5
>
> Diff: https://reviews.apache.org/r/41474/diff/
>
>
> Testing
> -------
>
> Viewed the markdown in a branch on GitHub: https://github.com/mesosphere/mesos/tree/defer_docs/3rdparty/libprocess
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 41474: Added documentation for `defer` in
libprocess README.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41474/#review111357
-----------------------------------------------------------
Patch looks great!
Reviews applied: [41474]
Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export COMPILER=gcc; ./support/docker_build.sh
- Mesos ReviewBot
On Dec. 19, 2015, 1:26 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41474/
> -----------------------------------------------------------
>
> (Updated Dec. 19, 2015, 1:26 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Joris Van Remoortere, and Neil Conway.
>
>
> Bugs: MESOS-3996
> https://issues.apache.org/jira/browse/MESOS-3996
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added documentation for `defer` in libprocess README.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/README.md c3f309a7dc1c94882c4cc97eeaf0736c2fca0ba5
>
> Diff: https://reviews.apache.org/r/41474/diff/
>
>
> Testing
> -------
>
> Viewed the markdown in a branch on GitHub: https://github.com/mesosphere/mesos/tree/defer_docs/3rdparty/libprocess
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 41474: Added documentation for `defer` in
libprocess README.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41474/#review111422
-----------------------------------------------------------
Patch looks great!
Reviews applied: [41474]
Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export COMPILER=gcc; ./support/docker_build.sh
- Mesos ReviewBot
On Dec. 20, 2015, 7:50 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41474/
> -----------------------------------------------------------
>
> (Updated Dec. 20, 2015, 7:50 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Joris Van Remoortere, and Neil Conway.
>
>
> Bugs: MESOS-3996
> https://issues.apache.org/jira/browse/MESOS-3996
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added documentation for `defer` in libprocess README.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/README.md c3f309a7dc1c94882c4cc97eeaf0736c2fca0ba5
>
> Diff: https://reviews.apache.org/r/41474/diff/
>
>
> Testing
> -------
>
> Viewed the markdown in a branch on GitHub: https://github.com/mesosphere/mesos/tree/defer_docs/3rdparty/libprocess
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 41474: Added documentation for `defer` in
libprocess README.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41474/#review111435
-----------------------------------------------------------
Ship it!
Thanks Greg! I made a few more tweaks given the complicated nature of these semantics and pushed this.
- Benjamin Hindman
On Dec. 20, 2015, 7:50 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41474/
> -----------------------------------------------------------
>
> (Updated Dec. 20, 2015, 7:50 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Joris Van Remoortere, and Neil Conway.
>
>
> Bugs: MESOS-3996
> https://issues.apache.org/jira/browse/MESOS-3996
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added documentation for `defer` in libprocess README.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/README.md c3f309a7dc1c94882c4cc97eeaf0736c2fca0ba5
>
> Diff: https://reviews.apache.org/r/41474/diff/
>
>
> Testing
> -------
>
> Viewed the markdown in a branch on GitHub: https://github.com/mesosphere/mesos/tree/defer_docs/3rdparty/libprocess
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 41474: Added documentation for `defer` in
libprocess README.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41474/
-----------------------------------------------------------
(Updated Dec. 20, 2015, 7:50 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Joris Van Remoortere, and Neil Conway.
Changes
-------
Fixed typo.
Bugs: MESOS-3996
https://issues.apache.org/jira/browse/MESOS-3996
Repository: mesos
Description
-------
Added documentation for `defer` in libprocess README.
Diffs (updated)
-----
3rdparty/libprocess/README.md c3f309a7dc1c94882c4cc97eeaf0736c2fca0ba5
Diff: https://reviews.apache.org/r/41474/diff/
Testing
-------
Viewed the markdown in a branch on GitHub: https://github.com/mesosphere/mesos/tree/defer_docs/3rdparty/libprocess
Thanks,
Greg Mann
Re: Review Request 41474: Added documentation for `defer` in
libprocess README.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41474/
-----------------------------------------------------------
(Updated Dec. 20, 2015, 7:46 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Joris Van Remoortere, and Neil Conway.
Changes
-------
Addressed comments.
Bugs: MESOS-3996
https://issues.apache.org/jira/browse/MESOS-3996
Repository: mesos
Description
-------
Added documentation for `defer` in libprocess README.
Diffs (updated)
-----
3rdparty/libprocess/README.md c3f309a7dc1c94882c4cc97eeaf0736c2fca0ba5
Diff: https://reviews.apache.org/r/41474/diff/
Testing
-------
Viewed the markdown in a branch on GitHub: https://github.com/mesosphere/mesos/tree/defer_docs/3rdparty/libprocess
Thanks,
Greg Mann
Re: Review Request 41474: Added documentation for `defer` in
libprocess README.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Dec. 19, 2015, 3:03 a.m., Neil Conway wrote:
> > 3rdparty/libprocess/README.md, line 211
> > <https://reviews.apache.org/r/41474/diff/2/?file=1172634#file1172634line211>
> >
> > What is this used for?
I just removed the boilerplate methods that are not relevant to the discussion at hand.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41474/#review111352
-----------------------------------------------------------
On Dec. 20, 2015, 7:50 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41474/
> -----------------------------------------------------------
>
> (Updated Dec. 20, 2015, 7:50 p.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Joris Van Remoortere, and Neil Conway.
>
>
> Bugs: MESOS-3996
> https://issues.apache.org/jira/browse/MESOS-3996
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added documentation for `defer` in libprocess README.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/README.md c3f309a7dc1c94882c4cc97eeaf0736c2fca0ba5
>
> Diff: https://reviews.apache.org/r/41474/diff/
>
>
> Testing
> -------
>
> Viewed the markdown in a branch on GitHub: https://github.com/mesosphere/mesos/tree/defer_docs/3rdparty/libprocess
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 41474: Added documentation for `defer` in
libprocess README.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41474/#review111352
-----------------------------------------------------------
3rdparty/libprocess/README.md (line 107)
<https://reviews.apache.org/r/41474/#comment171508>
"a future"
"When the event occurs,"
"all such" => "all"
3rdparty/libprocess/README.md (line 111)
<https://reviews.apache.org/r/41474/#comment171509>
In what sense is the "decision postponed"? By using `defer`, aren't I immediately deciding that the `dispatch` will be done asynchronously?
3rdparty/libprocess/README.md (line 149)
<https://reviews.apache.org/r/41474/#comment171510>
I might just say something like "Thus, fullfilling a future can result in executing callbacks either synchronously or asynchronously, depending on whether `defer` is used when the callback is registered."
(IMO talking about "synchronous execution of callbacks" makes things a bit more confusing.)
3rdparty/libprocess/README.md (line 151)
<https://reviews.apache.org/r/41474/#comment171511>
Weird sentence to end the paragraph. What is "this"? What is the pattern, and why is it useful?
3rdparty/libprocess/README.md (line 170)
<https://reviews.apache.org/r/41474/#comment171512>
Clarify what "it's" means here. Something like: "The important thing is that the process that registers the callback is (likely) different from the process that fulfills the future."
3rdparty/libprocess/README.md (line 173)
<https://reviews.apache.org/r/41474/#comment171513>
What "underlying implementation"?
3rdparty/libprocess/README.md (line 176)
<https://reviews.apache.org/r/41474/#comment171514>
Clarify what the data race is.
3rdparty/libprocess/README.md (line 178)
<https://reviews.apache.org/r/41474/#comment171515>
I'm a bit confused at this point. If the point is just to make sure that the callback is invoked in the context of a given process (e.g., `self()`) and not in the context of the process that satisfies the future, why can't I just use `dispatch()`?
3rdparty/libprocess/README.md (line 211)
<https://reviews.apache.org/r/41474/#comment171516>
What is this used for?
- Neil Conway
On Dec. 19, 2015, 1:26 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41474/
> -----------------------------------------------------------
>
> (Updated Dec. 19, 2015, 1:26 a.m.)
>
>
> Review request for mesos, Benjamin Hindman, Ben Mahler, Joris Van Remoortere, and Neil Conway.
>
>
> Bugs: MESOS-3996
> https://issues.apache.org/jira/browse/MESOS-3996
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added documentation for `defer` in libprocess README.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/README.md c3f309a7dc1c94882c4cc97eeaf0736c2fca0ba5
>
> Diff: https://reviews.apache.org/r/41474/diff/
>
>
> Testing
> -------
>
> Viewed the markdown in a branch on GitHub: https://github.com/mesosphere/mesos/tree/defer_docs/3rdparty/libprocess
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 41474: Added documentation for `defer` in
libprocess README.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41474/
-----------------------------------------------------------
(Updated Dec. 19, 2015, 1:26 a.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Joris Van Remoortere, and Neil Conway.
Changes
-------
Addressed comments.
Bugs: MESOS-3996
https://issues.apache.org/jira/browse/MESOS-3996
Repository: mesos
Description
-------
Added documentation for `defer` in libprocess README.
Diffs (updated)
-----
3rdparty/libprocess/README.md c3f309a7dc1c94882c4cc97eeaf0736c2fca0ba5
Diff: https://reviews.apache.org/r/41474/diff/
Testing
-------
Viewed the markdown in a branch on GitHub: https://github.com/mesosphere/mesos/tree/defer_docs/3rdparty/libprocess
Thanks,
Greg Mann