> On Feb. 16, 2015, 10:48 p.m., Jie Yu wrote: > > src/linux/cgroups.cpp, lines 1204-1210 > > <https://reviews.apache.org/r/31008/diff/1/?file=863519#file863519line1204> > > > > 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.
If we validate in the public / friended function by calling registerNotifier and closing the fd, can we just assume it'd always succeed in the constructor / initialize (which are void functions)? even if we could, it's still 2 pieces of the same code. I agree with being consistent and symetric, and to address the above leak, how about we put unregisterNotifier to destructor? On the construction side, we use a public creation function as the only construction path that provides explicit validation; the private constructor still takes the opened eventfd to avoid some code duplication? Does it sound like a better balance? IMHO more commonly client code forgets about proper closing after it has got the results than forgetting about getting the results -- i.e., it's probably more likely to forget to 'terminate' than to 'spawn'. Terminate requires an explict call; but compiler invokes destructor for us -- we could reduce the possible leak more for client code this way? Just my 2 cents. > On Feb. 16, 2015, 10:48 p.m., Jie Yu wrote: > > src/linux/cgroups.cpp, line 1215 > > <https://reviews.apache.org/r/31008/diff/1/?file=863519#file863519line1215> > > > > 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? Replied together below. > On Feb. 16, 2015, 10:48 p.m., Jie Yu wrote: > > src/linux/cgroups.cpp, lines 1307-1308 > > <https://reviews.apache.org/r/31008/diff/1/?file=863519#file863519line1307> > > > > 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. Yes, this is a bug. Thanks! See my comment below. > On Feb. 16, 2015, 10:48 p.m., Jie Yu wrote: > > src/linux/cgroups.cpp, line 1318 > > <https://reviews.apache.org/r/31008/diff/1/?file=863519#file863519line1318> > > > > Why do you need a Owned<Promise> here? Can it simply be: > > ``` > > Option<Promise<uint64>> promsie; > > ``` I don't think that'd compile since promise is not copyable and not assignable. More, Promise doesn't seem to have a 'reset' function to reset its associated future to PENDING again (could this be a useful patch, BTW?), which means a new promise has to be created to have a unset future. How about Result<Promise*> promise? We can have: None -- whenever there isn't a pending read and no errors have encounterred before. Also the initial value. Some -- Have a pending read, so we can reuse this promise. When the read completes successfully, future is set and promise is (deleted and) reset to none again. Error -- Set if a read is failed. The public listen functiion returns error from this point on and won't start a new listen. Leave it to the client code to clean this process up. Re: what if client discards the future returned by listen: How about change it to discard the current read if it hasn't completed or had an error? If it has, use the above logic. Basically future returned by listen only controls a single listen; client uses the process pointer from the constructor path to control the process' lifetime? - Chi ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31008/#review72643 ----------------------------------------------------------- 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 > >
