Re: 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/#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.
--- 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.
--- 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
