> On 2012-04-06 21:36:13, Benjamin Hindman wrote: > > src/slave/slave.hpp, line 101 > > <https://reviews.apache.org/r/4619/diff/3/?file=100676#file100676line101> > > > > While some of this patch might be getting thrown away, reusing > > something like this would be swell. Maybe sticking it in a "utils" like > > namespace for dealing with protobuf objects (i.e., > > mesos::protos::create<StatusUpdate>(...)).
talked offline. will do this in the slave restart branch. > On 2012-04-06 21:36:13, Benjamin Hindman wrote: > > src/slave/slave.cpp, line 75 > > <https://reviews.apache.org/r/4619/diff/3/?file=100677#file100677line75> > > > > Could also be in a generic "protos" namespace. see above. > On 2012-04-06 21:36:13, Benjamin Hindman wrote: > > src/slave/slave.cpp, lines 693-699 > > <https://reviews.apache.org/r/4619/diff/3/?file=100677#file100677line693> > > > > You should kill this and put executor->removeTask back in > > 'statusUpdate'. Since transitionLiveTask calls 'statusUpdate' the task will > > get removed. We only need the framework->updates.empty() check below to > > make sure we send all the updates. fixed > On 2012-04-06 21:36:13, Benjamin Hindman wrote: > > src/slave/slave.cpp, line 1002 > > <https://reviews.apache.org/r/4619/diff/3/?file=100677#file100677line1002> > > > > Put this back (see comment above). fixed > On 2012-04-06 21:36:13, Benjamin Hindman wrote: > > src/slave/slave.cpp, line 1536 > > <https://reviews.apache.org/r/4619/diff/3/?file=100677#file100677line1536> > > > > Just throw a comment here saying something along the lines of why > > calling 'executorExited' is the right thing to do (since before now > > 'executorExited' was only called from the isolation module when an executor > > actually exited). done > On 2012-04-06 21:36:13, Benjamin Hindman wrote: > > src/slave/slave.cpp, line 1447 > > <https://reviews.apache.org/r/4619/diff/3/?file=100677#file100677line1447> > > > > This would be *even* more clear if we did the logic of > > transitionLiveTask right here (I think you could do it without another if > > statement using ternary if), but since this is just a band-aide I'm not too > > picky. cool :) - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4619/#review6752 ----------------------------------------------------------- On 2012-04-06 19:02:24, Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4619/ > ----------------------------------------------------------- > > (Updated 2012-04-06 19:02:24) > > > Review request for mesos, Benjamin Hindman and John Sirois. > > > Summary > ------- > > This is a short-term fix, because this fix will be subsumed when with we > patch with the slave restart code. > > > Diffs > ----- > > src/master/master.cpp 4dc9ee0 > src/slave/slave.hpp 279bc7b > src/slave/slave.cpp 3358ec4 > > Diff: https://reviews.apache.org/r/4619/diff > > > Testing > ------- > > make check > > (All tests except external python test succeed) > > /src/examples/python/test-framework local > ./src/examples/python/test-framework: line 32: test: > /Users/vinod/workspace/apache/mesos/build/src/python/dist/mesos-0.0.1-py2.6-macosx-10.6-universal.egg: > binary operator expected > Traceback (most recent call last): > File > "/Users/vinod/workspace/apache/mesos/build/../src/examples/python/test_framework.py", > line 23, in <module> > import mesos > ImportError: No module named mesos > > > Thanks, > > Vinod > >
