incubator-mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Benjamin Hindman" <b...@berkeley.edu>
Subject Re: Review Request: Fix for properly sending TASK LOST status updates
Date Fri, 06 Apr 2012 17:25:43 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4619/#review6725
-----------------------------------------------------------



src/slave/slave.cpp
<https://reviews.apache.org/r/4619/#comment14756>

    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.



src/slave/slave.cpp
<https://reviews.apache.org/r/4619/#comment14644>

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



src/slave/slave.cpp
<https://reviews.apache.org/r/4619/#comment14590>

    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.



src/slave/slave.cpp
<https://reviews.apache.org/r/4619/#comment14757>

    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.


- Benjamin


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