You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Vinod Kone <vi...@gmail.com> on 2013/03/01 23:39:48 UTC
Re: Review Request: Slave Restart (Part Tres): StatusUpdatesManager
integration into the slave
> On Feb. 28, 2013, 10:11 p.m., Ben Mahler wrote:
> > src/slave/slave.hpp, line 139
> > <https://reviews.apache.org/r/7655/diff/11/?file=261368#file261368line139>
> >
> > Document the purpose of the optional executor argument, maybe use Option (with None() default) + CHECK_NOTNULL when the option is some?
Aah. I don't like options for pointers. But, added a comment.
> On Feb. 28, 2013, 10:11 p.m., Ben Mahler wrote:
> > src/slave/slave.cpp, line 76
> > <https://reviews.apache.org/r/7655/diff/11/?file=261369#file261369line76>
> >
> > why does it need to be a pointer at all?
The idea was to make it easy to inject a mock status update manager for testing (like we do isolation module). But, I never got around it :/
I can create this on a stack, if this bothers you.
> On Feb. 28, 2013, 10:11 p.m., Ben Mahler wrote:
> > src/slave/slave.cpp, line 610
> > <https://reviews.apache.org/r/7655/diff/11/?file=261369#file261369line610>
> >
> > I don't buy this comment, statusUpdate is functionally doing more than forwardUpdate.
> >
> > It is modifying the executor state, if found. Talking to the isolation module as well.
> >
> > Also, if statusUpdate forwarded for unknown frameworks we could call it here right? Sounds like we should take care of that TODO and eliminate the need for having _both_ statusUpdate and forwardUpdate, wouldn't that be much cleaner?
OK. After our discussions offline, I think the right thing to do here is to make statusUpdate() more permissive (i.e., forwarding updates irrespective of unknown frameworks/executors). We can revisit this strategy when there is persistent state at the master.
I am keeping forwardUpdate() instead of folding it statusUpdate() to make it easy to revert the logic in the future, when the master has persistence.
> On Feb. 28, 2013, 10:11 p.m., Ben Mahler wrote:
> > src/slave/slave.cpp, line 686
> > <https://reviews.apache.org/r/7655/diff/11/?file=261369#file261369line686>
> >
> > s/shut down/shutdown to be consistent
I think BenH doesn't like 'shutdown' in sentences. The log message above is talking about 'shutdown' framework message, which I think is ok.
> On Feb. 28, 2013, 10:11 p.m., Ben Mahler wrote:
> > src/slave/slave.cpp, line 791
> > <https://reviews.apache.org/r/7655/diff/11/?file=261369#file261369line791>
> >
> > Use "Failed" style message.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7655/#review17204
-----------------------------------------------------------
On Feb. 23, 2013, 8:11 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7655/
> -----------------------------------------------------------
>
> (Updated Feb. 23, 2013, 8:11 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> Integrated SUM into slave.
>
>
> This addresses bug MESOS-110.
> https://issues.apache.org/jira/browse/MESOS-110
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 38235157d45bdccb676e5c3241c21b585a6f8801
> src/Makefile.am c94736df660a25b58dc47c07d9c56c3c26152a66
> src/slave/gc.cpp fe22f9b557215514e3d432d36af9dc0c377c437b
> src/slave/paths.hpp fbf3fd84fb8f2590311b18d2afec2d2e0d30ef0a
> src/slave/slave.hpp e9f7b659ca2860501840b3d01e69915ebd162039
> src/slave/slave.cpp d4721c3eb51db87278d05f6fbe2eadb8a3a9b4dd
> src/slave/status_update_manager.hpp PRE-CREATION
> src/tests/master_tests.cpp e140f409bf6e651cc365b5fb8d064ceae1cd8ed8
> src/tests/status_update_manager_tests.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/7655/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>