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 21:36:13 GMT

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

Ship it!



src/slave/slave.hpp
<https://reviews.apache.org/r/4619/#comment14814>

    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>(...)).



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

    Could also be in a generic "protos" namespace.



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

    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.



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

    Put this back (see comment above).



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

    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.



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

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


- Benjamin


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


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