Re: Review Request 61147: Added Future::onAbandoned and Future::isAbandoned.

[Benjamin Mahler]

---
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)


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)


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)


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)


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)


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)


What happened here?



3rdparty/libprocess/src/tests/collect_tests.cpp
Lines 229-243 (original), 233-250 (patched)


Ditto here.



3rdparty/libprocess/src/tests/future_tests.cpp
Lines 558 (patched)


EXPECT not abandoned before you reset the promise?



3rdparty/libprocess/src/tests/metrics_tests.cpp
Lines 77-80 (original), 78-85 (patched)


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



3rdparty/libprocess/src/tests/process_tests.cpp
Line 176 (original), 177 (patched)


Commit separately?



3rdparty/libprocess/src/tests/process_tests.cpp
Line 216 (original), 215 (patched)


Commit this and the using statement separately?



3rdparty/libprocess/src/tests/shared_tests.cpp
Line 93 (original), 94 (patched)


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
> 
>

Review Request 61147: Added Future::onAbandoned and Future::isAbandoned.

[Benjamin Hindman]

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

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