> On Feb. 12, 2015, 9:06 p.m., Dominic Hamon wrote: > > src/linux/cgroups.cpp, line 1333 > > <https://reviews.apache.org/r/30545/diff/3/?file=862210#file862210line1333> > > > > i don't understand why the first reading is special in this case.
replyed the several comments above altogether below. > On Feb. 12, 2015, 9:06 p.m., Dominic Hamon wrote: > > src/linux/cgroups.cpp, line 2307 > > <https://reviews.apache.org/r/30545/diff/3/?file=862210#file862210line2307> > > > > you also create an EventListenerProcess in listen above. is that > > expected? I would have thought just one process would be created. that's for OOM-listening. Both use EventListenerProcess now. > On Feb. 12, 2015, 9:06 p.m., Dominic Hamon wrote: > > src/tests/cgroups_tests.cpp, line 1536 > > <https://reviews.apache.org/r/30545/diff/3/?file=862211#file862211line1536> > > > > can you not put explicit expectations on these event counters? it's 1 most of times, but have seen 2 once in while too in testing. I think 1 is from the direct memory reclaiming behavir triggered by my code; the occasional additional 1 is from the kernel thread kswapd that peridically reclaims memory. > On Feb. 12, 2015, 9:06 p.m., Dominic Hamon wrote: > > src/tests/cgroups_tests.cpp, line 1084 > > <https://reviews.apache.org/r/30545/diff/3/?file=862211#file862211line1084> > > > > looks like you can use the other helper method here and make this a > > one-liner. > > > > return stat().get(metric).get(); I can only do asserts in a void function, so used member field to 'detour'. > On Feb. 12, 2015, 9:06 p.m., Dominic Hamon wrote: > > src/linux/cgroups.cpp, line 2339 > > <https://reviews.apache.org/r/30545/diff/3/?file=862210#file862210line2339> > > > > this is very similar to EventListenerProcess::listen. > > > > i have the sense this API and implementation can be simplified quite a > > bit. How about make the difference more distinct by having listen fail if the event has happened? to make it more clear that listen is only intended for listening for an event not yet happened; counter gives you the sum. The main difference is listen doesn't want the future set until the first event arrived (OOM listening). counter is a request for 'how many have you read so far; nothing is a fine answer'. I also wanted to make a clear distinction at this level that 'none' means haven't read anything; 0 means have read something (was 0, but still means oom has happened). The new eventlistenprocess is a merge of the old one-time oom-listening and new continuous listening. The 2 APIS are brought from each. > On Feb. 12, 2015, 9:06 p.m., Dominic Hamon wrote: > > src/linux/cgroups.hpp, line 344 > > <https://reviews.apache.org/r/30545/diff/3/?file=862209#file862209line344> > > > > if the caller is responsible for the lifetime of the process, this > > should be a unique_ptr (or Owned) It would work for pressurelistener, which has the same lifetime of the underneath listening libprocss. But in the case of the oom listening, cgroups::listen spawns (spawn only takes a raw pointer) it off and returns. using a owned will have memory of the running libprocess freed. Try::get() returns a const ref, so I can' 'release' the raw pointer out of it. - Chi ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30545/#review72242 ----------------------------------------------------------- On Feb. 12, 2015, 8:08 p.m., Chi Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30545/ > ----------------------------------------------------------- > > (Updated Feb. 12, 2015, 8:08 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 > ------- > > cgroups: added support to listen on memory pressures. > > > Diffs > ----- > > src/Makefile.am 39ce858fd009f8aef388bd3d6b17aa4c885ff9c9 > src/linux/cgroups.hpp bf8efee05b995a37d4e6e7679a493b2d5238aa0b > src/linux/cgroups.cpp b6f75b14dea7609b90627eccdd33ef891ed9d21f > src/tests/cgroups_tests.cpp 9d50a47c939f5f136c85fdcc555459512c6f2259 > src/tests/memory_test_helper.hpp PRE-CREATION > src/tests/memory_test_helper.cpp PRE-CREATION > src/tests/memory_test_helper_main.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/30545/diff/ > > > Testing > ------- > > > Thanks, > > Chi Zhang > >
