Mailing-List: contact reviews-help@mesos.apache.org; run by ezmlm
Precedence: bulk
Reply-To: reviews@mesos.apache.org
Content-Type: multipart/alternative;
 boundary="===============7984874962819824019=="
MIME-Version: 1.0
Subject: Re: Review Request 61147: Added Future::onAbandoned and
 Future::isAbandoned.
From: Benjamin Mahler <bmahler@apache.org>
To: Benjamin Mahler <bmahler@apache.org>
Cc: Benjamin Hindman <benh@berkeley.edu>, mesos <reviews@mesos.apache.org>
Date: Sun, 30 Jul 2017 02:43:53 -0000
Message-ID: <20170730024353.15041.51456@reviews-vm2.apache.org>
Auto-Submitted: auto-generated
Sender: Benjamin Mahler <noreply@reviews.apache.org>
References: <20170727015520.51161.68740@reviews-vm2.apache.org>
In-Reply-To: <20170727015520.51161.68740@reviews-vm2.apache.org>
Reply-To: Benjamin Mahler <bmahler@apache.org>
archived-at: Sun, 30 Jul 2017 02:44:02 -0000

--===============7984874962819824019==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit


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


Fix it, then Ship it!


3rdparty/libprocess/include/process/future.hpp
Lines 84-88 (original), 84-88 (patched)
<https://reviews.apache.org/r/61147/#comment257451>

    This could use some discussion about abandonement, there doesn't seem to be any that talks about what abandonment is? (I guess you'll have to seed this since the current documentation is lacking). In particular, what I found helpful was to think about it as: a pending future can be abandoned (rather than abandoment being a terminal state). Maybe also a TODO about what we would do if we were designing future from scratch? Would we make abandonment a state that is captured by onAny?


3rdparty/libprocess/include/process/future.hpp
Line 1059 (original), 1108-1109 (patched)
<https://reviews.apache.org/r/61147/#comment257452>

    A high level comment about what propagating means here would be great! (maybe an example to make it even more clear?)


3rdparty/libprocess/include/process/queue.hpp
Lines 57-69 (original), 57-66 (patched)
<https://reviews.apache.org/r/61147/#comment257453>

    Commit this separately? I think it was written the original way to avoid needing an UNREACHABLE?


3rdparty/libprocess/include/process/queue.hpp
Line 59 (original), 57 (patched)
<https://reviews.apache.org/r/61147/#comment257455>

    It's funny, now that dispatching is lock free, I suspect that the Process-based implementation of Pipe would be faster than the locking approach! This is potentially true across other components, like process::Queue!


3rdparty/libprocess/src/http.cpp
Lines 422-441 (original), 422-438 (patched)
<https://reviews.apache.org/r/61147/#comment257454>

    Ditto here w.r.t. committing separately. I also thought this was written the original way to avoid UNREACHABLE.


3rdparty/libprocess/src/tests/collect_tests.cpp
Lines 114-127 (original), 114-131 (patched)
<https://reviews.apache.org/r/61147/#comment257456>

    What happened here?


3rdparty/libprocess/src/tests/collect_tests.cpp
Lines 229-243 (original), 233-250 (patched)
<https://reviews.apache.org/r/61147/#comment257457>

    Ditto here.


3rdparty/libprocess/src/tests/future_tests.cpp
Lines 558 (patched)
<https://reviews.apache.org/r/61147/#comment257458>

    EXPECT not abandoned before you reset the promise?


3rdparty/libprocess/src/tests/metrics_tests.cpp
Lines 77-80 (original), 78-85 (patched)
<https://reviews.apache.org/r/61147/#comment257459>

    Just curious, does it matter that the pending future is abandoned?


3rdparty/libprocess/src/tests/process_tests.cpp
Line 176 (original), 177 (patched)
<https://reviews.apache.org/r/61147/#comment257460>

    Commit separately?


3rdparty/libprocess/src/tests/process_tests.cpp
Line 216 (original), 215 (patched)
<https://reviews.apache.org/r/61147/#comment257461>

    Commit this and the using statement separately?


3rdparty/libprocess/src/tests/shared_tests.cpp
Line 93 (original), 94 (patched)
<https://reviews.apache.org/r/61147/#comment257462>

    Commit this and the using statement separately?


- Benjamin Mahler


On July 27, 2017, 1:55 a.m., Benjamin Hindman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61147/
> -----------------------------------------------------------
> 
> (Updated July 27, 2017, 1:55 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added Future::onAbandoned and Future::isAbandoned.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/future.hpp cce950509f58022e79bb51a6e72ea1a005b9cb50 
>   3rdparty/libprocess/include/process/queue.hpp ab08e30df742412f22a06202526f8b55350ed435 
>   3rdparty/libprocess/src/http.cpp a4d71fb6c345d3c7a7611004830f6c2c0fbf6046 
>   3rdparty/libprocess/src/tests/collect_tests.cpp 155e0bb75cf723a0a6c29020f9f767e3ba3d7401 
>   3rdparty/libprocess/src/tests/future_tests.cpp 0c8725b9a5e64aaac6e3979e450a11e84f9bd45e 
>   3rdparty/libprocess/src/tests/metrics_tests.cpp 161ca0dc7aea526d450d71a80839d8cc075aaa31 
>   3rdparty/libprocess/src/tests/process_tests.cpp ed11909a2a5e3214fa974bdea098f4057cea9666 
>   3rdparty/libprocess/src/tests/shared_tests.cpp 2a2ffe76b7b7ce016b559de7b5d3a28a06f422ef 
> 
> 
> Diff: https://reviews.apache.org/r/61147/diff/1/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>


--===============7984874962819824019==--