----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31008/#review72825 -----------------------------------------------------------
Ship it! Looking good! Only a few nits. src/linux/cgroups.cpp <https://reviews.apache.org/r/31008/#comment118917> "and assumes that arguments are validated." s/public function/public interface/ src/linux/cgroups.cpp <https://reviews.apache.org/r/31008/#comment118918> ```s/eventfd_/fd``` src/linux/cgroups.cpp <https://reviews.apache.org/r/31008/#comment118920> Hum, what if `registerNotifier` fails? This will be a segfault! I would suggest reverting the change and making `eventfd` an `Option<int>`. src/linux/cgroups.cpp <https://reviews.apache.org/r/31008/#comment118923> Not your fault, but let's fail the promise here ``` promise.get()->fail("Event listener is terminating"); ``` src/linux/cgroups.cpp <https://reviews.apache.org/r/31008/#comment118924> This comment is confusing. I would suggest just kill it. src/linux/cgroups.cpp <https://reviews.apache.org/r/31008/#comment118927> insert one blank line above. Also, i would suggest the following style: ``` if (reading.isReady() && reading.get() == sizeof(data)) { promise.get()->set(data); // ... promise = None(); return; } // Error cases. ``` src/linux/cgroups.cpp <https://reviews.apache.org/r/31008/#comment118928> You don't need std:: prefix here as this is in cpp. Please do a sweep to fix all such code in this file. Thanks! Also, see my comments below. src/linux/cgroups.cpp <https://reviews.apache.org/r/31008/#comment118929> I would suggest simply set the error directly: ``` if (reading.isDiscarded()) { error = Error("..."); } else if (...) { error = Error("..."); } else { error = Error("..."); } promise.get()->fail(error.get().message); ``` src/linux/cgroups.cpp <https://reviews.apache.org/r/31008/#comment118932> "Read less than expected: expect yyy bytes and actual xxx bytes" src/linux/cgroups.cpp <https://reviews.apache.org/r/31008/#comment118922> Why reordering these fields? src/linux/cgroups.cpp <https://reviews.apache.org/r/31008/#comment118921> See my above comments, please revert this change. src/linux/cgroups.cpp <https://reviews.apache.org/r/31008/#comment118936> Style nits. Align them probably: ``` future .onDiscard(lambda::bind( ...) .onAny(lambda::bind( ...); ``` src/linux/cgroups.cpp <https://reviews.apache.org/r/31008/#comment118934> One more blank line - Jie Yu On Feb. 17, 2015, 8:10 p.m., Chi Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31008/ > ----------------------------------------------------------- > > (Updated Feb. 17, 2015, 8:10 p.m.) > > > Review request for mesos, Dominic Hamon, Ian Downes, and Jie Yu. > > > Bugs: mesos-2136 > https://issues.apache.org/jira/browse/mesos-2136 > > > Repository: mesos > > > Description > ------- > > Refactored EventListener to allow for continuous monitoring of an event. > > > Diffs > ----- > > src/linux/cgroups.hpp bf8efee05b995a37d4e6e7679a493b2d5238aa0b > src/linux/cgroups.cpp b6f75b14dea7609b90627eccdd33ef891ed9d21f > > Diff: https://reviews.apache.org/r/31008/diff/ > > > Testing > ------- > > > Thanks, > > Chi Zhang > >
