mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gaston Kleiman <gas...@mesosphere.io>
Subject Re: Review Request 65962: Avoided copying `Owned` pointers in the default executor.
Date Tue, 20 Mar 2018 21:51:19 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message