incubator-mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Vinod Kone" <vinodk...@gmail.com>
Subject Re: Review Request: Fix for properly sending TASK LOST status updates
Date Fri, 06 Apr 2012 18:59:51 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message