> On March 20, 2018, 6:05 a.m., Alexander Rukletsov wrote: > > src/launcher/default_executor.cpp > > Lines 943-958 (original) > > <https://reviews.apache.org/r/65962/diff/3/?file=1978911#file1978911line943> > > > > I'd rather see such code shuffling in a separate patch. It is not > > strictly equivalent (for example, before if the container is the last one, > > shutdown is initiated immediately, while now if-blob will be executed first.
I am afraid that this shuffling is necessary because the change in line 774 (which clearly belongs in this patch) implies that the `Container` object will be destroyed as soon as it is removed from the `containers` map. The rest of the method uses the object, so we have to delay the removal of the object until it is no longer needed. Note that the if-blob that you mention initiates (if necessary) the killing of the tasks in the same task group as the task that exited. You are right, the code after the change is not strictly equivalent; the if-blob will no longer be skipped when the task that exited was the only running task. However, in that case, the statement in line 969 will evaluate to `true`, making it a NOOP, and thus semantically equivalent. - Gaston ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65962/#review199541 ----------------------------------------------------------- On March 19, 2018, 9:36 a.m., Gaston Kleiman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65962/ > ----------------------------------------------------------- > > (Updated March 19, 2018, 9:36 a.m.) > > > Review request for mesos, Alexander Rukletsov, Benjamin Bannier, and Joseph > Wu. > > > Repository: mesos > > > Description > ------- > > `Owned` pointers are copied in multiple places of the default executor. > This violates the semantic of owned pointers and works only because > `Owned` is currently implemented with `shared_ptr`, it would otherwise > lead to double-freeing the pointers. > > This patch changes those places to use references to the original > `Owned` objects or raw pointers instead of copies. > > > Diffs > ----- > > src/launcher/default_executor.cpp 906836f3b8e0af79d7c61f90fd8a95f193b26e84 > > > Diff: https://reviews.apache.org/r/65962/diff/3/ > > > Testing > ------- > > `sudo bin/mesos-tests.sh --gtest_filter="*Default*"` on GNU/Linux > > > Thanks, > > Gaston Kleiman > >
