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 Mon, 09 Apr 2012 16:44:00 GMT


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


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