----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31008/#review72643 -----------------------------------------------------------
Thanks Chi! This is my first pass. Will do a second pass one the following issues are addressed. src/linux/cgroups.cpp <https://reviews.apache.org/r/31008/#comment118687> We usually call a libprocess process a 'process'. So please revert this change. src/linux/cgroups.cpp <https://reviews.apache.org/r/31008/#comment118703> To be consistent and symetric, putting `registerNotifier` in `initialize` is better than putting it here. What if the caller creates the process but forgets to spawn it? Do you leak an opened event file? I would rather keep the existing constructor. We can hide the public constructor and make those exposed public functions friend. Validations can go into those public exposed functions. src/linux/cgroups.cpp <https://reviews.apache.org/r/31008/#comment118704> OK, let's document the semantics for this public function. The semantics you have is slightly different than I initially thought, but I think you sematics is simpler and easy to implement. Your semantics is actually waiting for the *next* event to occur and returns the value read from the event file. Calling multiple `read` returns the same future which will be satisfied when the *next* event occurs. Because of that, I think we probably should rename `read` to `listen`. What do you think? Also, what is the discard semantics. What if the user discard the returned future, what's gonna happen? src/linux/cgroups.cpp <https://reviews.apache.org/r/31008/#comment118709> See my comments below. No need to print path and args as they should be printed by the caller. src/linux/cgroups.cpp <https://reviews.apache.org/r/31008/#comment118710> Ditto.. src/linux/cgroups.cpp <https://reviews.apache.org/r/31008/#comment118708> Thanks for expanding the error message. However, you don't need to print the path and args because these are caller's responsibility. src/linux/cgroups.cpp <https://reviews.apache.org/r/31008/#comment118707> So you reset the promise once a `read` has finished, even if it has failed. This is problematic because what if the previous read only returns a partial result (i.e., reading.get() != sizeof(data)), will that affect the following reads? I guess the behavior is undefined if a previous read returns a failure. So you probably want to return failure if there is a failure in the previous read. src/linux/cgroups.cpp <https://reviews.apache.org/r/31008/#comment118706> Why do you need a Owned<Promise> here? Can it simply be: ``` Option<Promise<uint64>> promsie; ``` src/linux/cgroups.cpp <https://reviews.apache.org/r/31008/#comment118711> You should not call a method in a process directly. Please use dispatch! - Jie Yu On Feb. 13, 2015, 6:07 p.m., Chi Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31008/ > ----------------------------------------------------------- > > (Updated Feb. 13, 2015, 6:07 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 > >
