> On March 7, 2013, 6:46 p.m., Vinod Kone wrote: > >
Thanks for the review! > On March 7, 2013, 6:46 p.m., Vinod Kone wrote: > > src/tests/isolator_tests.cpp, line 167 > > <https://reviews.apache.org/r/9797/diff/1/?file=267815#file267815line167> > > > > I see now why you set the 'id' in utils.hpp. > > > > The right way to do this is, do an EXPECT on scheduler's registered > > call and grab the framework id. Ah, I see, seems easier if we didn't test that logic everywhere and made it easier on test writers by having the default include an id =/ > On March 7, 2013, 6:46 p.m., Vinod Kone wrote: > > src/tests/isolator_tests.cpp, line 182 > > <https://reviews.apache.org/r/9797/diff/1/?file=267815#file267815line182> > > > > s/codes/code/ Changed to "a non-zero exit code". > On March 7, 2013, 6:46 p.m., Vinod Kone wrote: > > src/tests/utils.hpp, line 154 > > <https://reviews.apache.org/r/9797/diff/1/?file=267816#file267816line154> > > > > when a framework doesn't have an id set, the id is given by the master > > when it first registers. when it fails over, a framework uses this id to > > re-register with master. > > > > tldr; revert this. Ah, I see. > On March 7, 2013, 6:46 p.m., Vinod Kone wrote: > > src/tests/environment.cpp, line 108 > > <https://reviews.apache.org/r/9797/diff/1/?file=267814#file267814line108> > > > > Why the TODO instead of fixing it? This is going to break Jenkins if it > > gets submittted asis? > > > > Also, call it CgroupsIsolationModule till we get around to renaming it. Agreed, I just wanted to rebase this off of: https://reviews.apache.org/r/9798/ Benh is currently in the process of a rename. > On March 7, 2013, 6:46 p.m., Vinod Kone wrote: > > src/tests/isolator_tests.cpp, lines 173-175 > > <https://reviews.apache.org/r/9797/diff/1/?file=267815#file267815line173> > > > > Umm. This doesn't really test that you are collecting usage from the > > executor process tree :/ > > > > In other words, this test wouldn't fail even if you would've run it > > with the previous code? You're right, thanks for checking me on that! This test does fail before the descendant usage was aggregated, but only because the system time was *sometimes* zero. So, I'm guilty. I've made this more robust (for cpu times), thanks for the push! Now, without child process usage: cpu_user_time: 0.01 cpu_system_time: 0.01 memory_rss: 10825728 With child process usage: cpu_user_time: 0.23 cpu_system_time: 0.28 memory_rss: 13058048 - Ben ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/9797/#review17557 ----------------------------------------------------------- On March 7, 2013, 3:20 a.m., Ben Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/9797/ > ----------------------------------------------------------- > > (Updated March 7, 2013, 3:20 a.m.) > > > Review request for mesos, Benjamin Hindman and Vinod Kone. > > > Description > ------- > > Finally some non-external isolation module tests! > > > Diffs > ----- > > src/Makefile.am f7242a7fb55de3d1df4930126345274124331d57 > src/tests/environment.cpp 7c8c2c60ec77c732bf32292088cf5f4cb879ed89 > src/tests/isolator_tests.cpp PRE-CREATION > src/tests/utils.hpp 0d4edad4787f92f86b1486b6b510f46a62d7d541 > > Diff: https://reviews.apache.org/r/9797/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Ben Mahler > >
