> On 2012-04-06 17:25:43, Benjamin Hindman wrote: > > src/slave/slave.cpp, line 1327 > > <https://reviews.apache.org/r/4619/diff/2/?file=98738#file98738line1327> > > > > Rather than introduce a new function 'sendStatusUpdate' it seems like > > we should really just leverage the existing 'statusUpdate' function ... > > there is no reason why you can't call that places you'd otherwise be > > calling 'sendStatusUpdate', and then the logic for dealing with status > > updates is all in one place.
agreed. i wanted to pull the creation of status updates into a helper. this helper turned out to do more than creating status updates. fixed. > On 2012-04-06 17:25:43, Benjamin Hindman wrote: > > src/slave/slave.cpp, line 1344 > > <https://reviews.apache.org/r/4619/diff/2/?file=98738#file98738line1344> > > > > For example, this code is confusing here. It seems like the invariant > > when calling this function is that 'framework' and 'executor' exist. And > > yet, we still look them up, and never end up using 'executor'. fixed > On 2012-04-06 17:25:43, Benjamin Hindman wrote: > > src/slave/slave.cpp, line 1432 > > <https://reviews.apache.org/r/4619/diff/2/?file=98738#file98738line1432> > > > > In general putting something like this in this "helper" function gives > > it a nasty side-effect, and in particular, this means that if an executor > > was running 3 tasks we'll send ExitedExecutorMessage to the master 3 times. > > Even though this patch is just a band-aid *and* the master should be able > > to handle those "extra" messages, this needs to be fixed. fixed > On 2012-04-06 17:25:43, Benjamin Hindman wrote: > > src/slave/slave.cpp, line 1477 > > <https://reviews.apache.org/r/4619/diff/2/?file=98738#file98738line1477> > > > > I don't think 'transitionLiveTasks' actually transitions the state of > > the task because 'sendStatusUpdate' doesn't. But 'statusUpdate' does! > > Again, it seems much cleaner just to create StatusUpdate objects here and > > simply call 'statusUpdate' as though it had come from the executor. fixed - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4619/#review6725 ----------------------------------------------------------- On 2012-04-05 18:19:14, Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4619/ > ----------------------------------------------------------- > > (Updated 2012-04-05 18:19:14) > > > 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 > >
