----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65449/#review196665 -----------------------------------------------------------
partial review. src/master/master.hpp Lines 867-871 (patched) <https://reviews.apache.org/r/65449/#comment276422> s/toLaunchExecutor/launchExecutor/ More importnatly, we don't typically use this pattern of passing a pointer and changing its value for `int` like fields. See below. src/master/master.cpp Line 4914 (original), 4918 (patched) <https://reviews.apache.org/r/65449/#comment276423> How about we figure whether to launch an executor or not here and then call `addTask`? src/master/master.cpp Lines 4986 (patched) <https://reviews.apache.org/r/65449/#comment276424> This comes out as `0 executor launch` or `1 executor launch` which reads weird. how about: ``` LOG(INFO) << "Launching task " << task.task_id() << " of framework " << *framework << " with resources " << task.resources() << " on agent " << *slave << " on " << (message.launch_executor() ? " new executor" << " existing executor"); ``` src/master/master.cpp Line 5143 (original), 5151-5160 (patched) <https://reviews.apache.org/r/65449/#comment276426> ditto. see above. do the check for executor launch outside the for loop. src/master/master.cpp Lines 5177-5179 (patched) <https://reviews.apache.org/r/65449/#comment276427> see above. should be set outside the loop. src/master/master.cpp Lines 5193-5194 (patched) <https://reviews.apache.org/r/65449/#comment276428> see above. src/slave/slave.hpp Lines 170 (patched) <https://reviews.apache.org/r/65449/#comment276429> s/toLaunchExecutor/launchExecutor/ src/slave/slave.hpp Lines 180 (patched) <https://reviews.apache.org/r/65449/#comment276430> ditto. here and below. src/slave/slave.cpp Lines 1806 (patched) <https://reviews.apache.org/r/65449/#comment276431> I think old masters didn't used to set this. If 1.x masters do it then we can do a CHECK. src/slave/slave.cpp Lines 1816 (patched) <https://reviews.apache.org/r/65449/#comment276432> you can put fraemworkInfo in the previous line? src/slave/slave.cpp Lines 2065 (patched) <https://reviews.apache.org/r/65449/#comment276433> do you need the temporary variable? - Vinod Kone On Feb. 1, 2018, 2:03 a.m., Meng Zhu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65449/ > ----------------------------------------------------------- > > (Updated Feb. 1, 2018, 2:03 a.m.) > > > Review request for mesos, Benjamin Mahler, Chun-Hung Hsiao, and Vinod Kone. > > > Bugs: MESOS-1720 > https://issues.apache.org/jira/browse/MESOS-1720 > > > Repository: mesos > > > Description > ------- > > Master relies on `ExitedExecutorMessage` from the agent to recycle > executor entry. However, this message won't be sent if the executor > never actually launched (due to transient error), leaving executor > info on the master lingering and resource claimed. > See MESOS-1720. > > This patch fixes this issue by sending the `ExitedExecutorMessage` > from the agent if the executor is never launched. And by > setting a new field `launch_executor` in the RunTask(Group)Message, > the master is able to control the executor creation on the agent. > > > Diffs > ----- > > src/master/master.hpp b434d2398b8815811345b6586ca586d2025cb2a2 > src/master/master.cpp b97ebae6ebfd8ae0f73e617d0c55e140b9c3fce7 > src/slave/slave.hpp 09c01ebd1b5e8008ba9e7d412042f1db76a2c5a5 > src/slave/slave.cpp a6a5c93ab2d541c870cb52587495de20ed5ac1f4 > src/tests/mock_slave.hpp 29ce7140501888d95d5f2d6c26b752ad276b484a > src/tests/mock_slave.cpp 8357edc7b3a35624c813eccb9ecca9d3b5dbe07c > src/tests/slave_tests.cpp f393a8433a984267adc4db307ef07fcbafd1e62f > > > Diff: https://reviews.apache.org/r/65449/diff/1/ > > > Testing > ------- > > make check > Dedicated test in #65448 > > > Thanks, > > Meng Zhu > >
