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