You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2014/10/09 23:39:36 UTC
Review Request 26525: Moved executor checkpointing code from the
Executor constructor.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26525/
-----------------------------------------------------------
Review request for mesos, Ben Mahler and Vinod Kone.
Repository: mesos-git
Description
-------
There are two places where 'new Executor' is called:
1) launchExecutor
2) recoverExecutor
For 2), we don't need checkpointing. Therefore, putting checkpointing code in Executor constructor and use state != RECOVERING to disginguish is not explicit and confusing.
Diffs
-----
src/slave/slave.hpp 28697102047b972ecb3b6b627ee089b430549fc0
src/slave/slave.cpp e56dcbd80114730949a0d4b553470802a4d38281
Diff: https://reviews.apache.org/r/26525/diff/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 26525: Moved executor checkpointing code from the
Executor constructor.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26525/#review56179
-----------------------------------------------------------
src/slave/slave.cpp
<https://reviews.apache.org/r/26525/#comment96501>
I think it is weird to call a checkpoint*() function that is a no-op if it is not checkpointing. It is my bad, because I realize I did the same mistake with checkpointTask().
Can you fix both checkpointExecutor() and checkpointTask() to only be called when we are checkpointing?
src/slave/slave.cpp
<https://reviews.apache.org/r/26525/#comment96502>
This should be a CHECK once you fix the callers.
src/slave/slave.cpp
<https://reviews.apache.org/r/26525/#comment96504>
Change this to VLOG(1).
src/slave/slave.cpp
<https://reviews.apache.org/r/26525/#comment96503>
ditto. this should be a CHECK.
- Vinod Kone
On Oct. 9, 2014, 9:39 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26525/
> -----------------------------------------------------------
>
> (Updated Oct. 9, 2014, 9:39 p.m.)
>
>
> Review request for mesos, Ben Mahler and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> There are two places where 'new Executor' is called:
> 1) launchExecutor
> 2) recoverExecutor
>
> For 2), we don't need checkpointing. Therefore, putting checkpointing code in Executor constructor and use state != RECOVERING to disginguish is not explicit and confusing.
>
>
> Diffs
> -----
>
> src/slave/slave.hpp 28697102047b972ecb3b6b627ee089b430549fc0
> src/slave/slave.cpp e56dcbd80114730949a0d4b553470802a4d38281
>
> Diff: https://reviews.apache.org/r/26525/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 26525: Moved executor checkpointing code from the
Executor constructor.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26525/#review56061
-----------------------------------------------------------
Bad patch!
Reviews applied: [26525]
Failed command: git apply --index 26525.patch
Error:
error: patch failed: src/slave/slave.cpp:3935
error: src/slave/slave.cpp: patch does not apply
- Mesos ReviewBot
On Oct. 9, 2014, 9:39 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26525/
> -----------------------------------------------------------
>
> (Updated Oct. 9, 2014, 9:39 p.m.)
>
>
> Review request for mesos, Ben Mahler and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> There are two places where 'new Executor' is called:
> 1) launchExecutor
> 2) recoverExecutor
>
> For 2), we don't need checkpointing. Therefore, putting checkpointing code in Executor constructor and use state != RECOVERING to disginguish is not explicit and confusing.
>
>
> Diffs
> -----
>
> src/slave/slave.hpp 28697102047b972ecb3b6b627ee089b430549fc0
> src/slave/slave.cpp e56dcbd80114730949a0d4b553470802a4d38281
>
> Diff: https://reviews.apache.org/r/26525/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 26525: Moved executor checkpointing code from the
Executor constructor.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26525/#review56219
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
On Oct. 10, 2014, 8:40 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26525/
> -----------------------------------------------------------
>
> (Updated Oct. 10, 2014, 8:40 p.m.)
>
>
> Review request for mesos, Ben Mahler and Vinod Kone.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> There are two places where 'new Executor' is called:
> 1) launchExecutor
> 2) recoverExecutor
>
> For 2), we don't need checkpointing. Therefore, putting checkpointing code in Executor constructor and use state != RECOVERING to disginguish is not explicit and confusing.
>
>
> Diffs
> -----
>
> src/slave/slave.hpp 76d505c
> src/slave/slave.cpp cb37599
>
> Diff: https://reviews.apache.org/r/26525/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 26525: Moved executor checkpointing code from the
Executor constructor.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26525/
-----------------------------------------------------------
(Updated Oct. 10, 2014, 8:40 p.m.)
Review request for mesos, Ben Mahler and Vinod Kone.
Changes
-------
Vinod's comments.
Repository: mesos-git
Description
-------
There are two places where 'new Executor' is called:
1) launchExecutor
2) recoverExecutor
For 2), we don't need checkpointing. Therefore, putting checkpointing code in Executor constructor and use state != RECOVERING to disginguish is not explicit and confusing.
Diffs (updated)
-----
src/slave/slave.hpp 76d505c
src/slave/slave.cpp cb37599
Diff: https://reviews.apache.org/r/26525/diff/
Testing
-------
make check
Thanks,
Jie Yu