mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anindya Sinha <anindya_si...@apple.com>
Subject Re: Review Request 45961: Support sharing of resources through reference counting of resources.
Date Sat, 13 Aug 2016 00:58:15 GMT


> On Aug. 12, 2016, 5:44 p.m., Jiang Yan Xu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 628
> > <https://reviews.apache.org/r/45961/diff/12/?file=1458444#file1458444line628>
> >
> >     Move the check blow into the loop?
> >     
> >     ```
> >     // Master would only allow additional instances of shared resources to be allocated.
> >     CHECK_EQ(Resources(task.resources()).shared(), Resources(task.resources()));
> >     
> >     // Master only uses pass these resources through task resources.
> >     CHECK_TRUE(Resources(task.executor().resources()).empty());
> >     ```

As discussed, there is no way for master to *not* know the intention of the allocator pertaining
to `LAUNCH` operations in `updateAllocation()`. Therefore, we only populate the additional
shared resources in `TaskInfo::resources` and let allocator retrieve those additional resources.
The rest of the fields are not important to the master and allocator.


> On Aug. 12, 2016, 5:44 p.m., Jiang Yan Xu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 668
> > <https://reviews.apache.org/r/45961/diff/12/?file=1458444#file1458444line668>
> >
> >     Below this line:
> >     
> >     ```
> >     // A shared resource must have already been allocated to this framework for
it to be eligile for allocation for additional instances.
> >     foreach (const Resource& resource, additional) {
> >       CHECK(frameworkAllocation.contains(resource));
> >     }
> >     ```

This should be equivalent right?
```
CHECK(frameworkAllocation.contains(additional));
```


> On Aug. 12, 2016, 5:44 p.m., Jiang Yan Xu wrote:
> > src/master/allocator/sorter/drf/sorter.cpp, line 163
> > <https://reviews.apache.org/r/45961/diff/12/?file=1458446#file1458446line163>
> >
> >     s/, and/; / is more grammatrically correct?

Well, that is debatable but updated nevertheless.


> On Aug. 12, 2016, 5:44 p.m., Jiang Yan Xu wrote:
> > src/master/master.cpp, line 3881
> > <https://reviews.apache.org/r/45961/diff/12/?file=1458449#file1458449line3881>
> >
> >     s/needs/need/ (because of "copies")

Same comment as the one before?


> On Aug. 12, 2016, 5:44 p.m., Jiang Yan Xu wrote:
> > src/master/master.cpp, line 3886
> > <https://reviews.apache.org/r/45961/diff/12/?file=1458449#file1458449line3886>
> >
> >     I suggested this paragraph but I feel the following is more clear, what do you
think?
> >     
> >     ```
> >     TODO(anindya_sinha): The solution is temporary as it should be up to the allocator
to accept the request and allocate these additional resources. However it will only be possible
when the allocator becomes the entity that accepts and manages offers (MESOS-4553). Then we
should move the logic that allocates additional shared resources when accepting the LAUNCH
call to the allocator.
> >     ```

Ok. Changed to this version.


> On Aug. 12, 2016, 5:44 p.m., Jiang Yan Xu wrote:
> > src/master/validation.cpp, line 1170
> > <https://reviews.apache.org/r/45961/diff/12/?file=1458451#file1458451line1170>
> >
> >     This would involve copying too. 
> >     
> >     I think we can directly use these resources without copying.
> >     
> >     ```
> >     if (task.resources().contains(volume)) {
> >       return Error(...);
> >     }
> >     
> >     if (task.has_executor() && 
> >         task.executor().resources().contains(volume)) {
> >       return Error(...);
> >     }
> >     ```
> >     
> >     Note the singular volume here for the same reason as noted previously.

I do not think we can avoid a copy since `task.resources()` or `task.executor().resources()`
returns a  `google::protobuf::RepeatedPtrField<mesos::Resource>`. We need a `Resources`
object from `google::protobuf::RepeatedPtrField<mesos::Resource>` to do a `contains()`.


> On Aug. 12, 2016, 5:44 p.m., Jiang Yan Xu wrote:
> > src/master/validation.cpp, lines 1166-1167
> > <https://reviews.apache.org/r/45961/diff/12/?file=1458451#file1458451line1166>
> >
> >     This could result in copying.
> >     
> >     The alternative should be able to avoid it.
> >     
> >     ```
> >     foreachvalue(const hashmap<TaskID, TaskInfo>& tasks, pendingTasks)
{
> >       foreachvalue (const TaskInfo& task, tasks) {
> >       ...
> >       }
> >     }
> >     ```

As discussed, we get into a compile issue which we need to further investigate. In the meantime,
I will keep it as is.


- Anindya


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45961/#review144723
-----------------------------------------------------------


On Aug. 13, 2016, 12:57 a.m., Anindya Sinha wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45961/
> -----------------------------------------------------------
> 
> (Updated Aug. 13, 2016, 12:57 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Joris Van Remoortere, Michael Park, and Jiang
Yan Xu.
> 
> 
> Bugs: MESOS-4431
>     https://issues.apache.org/jira/browse/MESOS-4431
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> o Each shared resource is accouted via its share count. This count is
>   updated based on the resource operations (such as add and subtract)
>   in various scenarios such as task launch and terminate at multiple
>   modules such as master, allocator, sorter, etc.
> o Only allow DESTROY if there are no running or pending tasks using
>   the volume. However, if the volume is in a pending offer to one or
>   more frameworks, rescind that offer before processing the DESTROY.
> o To allow multiple tasks to be launched in the same ACCEPT call
>   using the same shared resource, we update the allocator and
>   sorter with additional copies of shared resources to reflect the
>   true shared count of allocated shared resources.
> 
> 
> Diffs
> -----
> 
>   src/common/resources.cpp 6b7af9179121efbdc5c29484eb042778bcea8288 
>   src/common/resources_utils.cpp 8e881a09500b0966b577e514077460f724868a8d 
>   src/master/allocator/mesos/hierarchical.hpp bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05

>   src/master/allocator/mesos/hierarchical.cpp 234ef98529964a0b6d3f132426a6c8ccbb1263ee

>   src/master/allocator/sorter/drf/sorter.hpp bc6bfb2d5d3b32d55be055a0514861b4e7d889bb

>   src/master/allocator/sorter/drf/sorter.cpp ac85b327fc33d34246788e6a8c8bf5a486c61434

>   src/master/http.cpp 52dd80b856cf2317c0b73ba54bf501696786088d 
>   src/master/master.hpp 6decff6f4b9c3434de030fd5c06df4c683a7abad 
>   src/master/master.cpp 0bd1a3490a86fede86a3f5f62ce4745b65aae258 
>   src/master/validation.hpp 43d876b00511d4ef652c2685d4950c994b5eef31 
>   src/master/validation.cpp af18e5aef3be59830b0a7b0235bbc739dba1029c 
>   src/tests/master_validation_tests.cpp 9eb82a569f7c48caa96d4d54a93b199ccac74385 
>   src/tests/sorter_tests.cpp 821e30d6574b045d25d4de4f7c3b8ac5346d3002 
>   src/v1/resources.cpp 03ee0cb0bb5abe7fc1ae4cb47c5b6dbcd8d11998 
> 
> Diff: https://reviews.apache.org/r/45961/diff/
> 
> 
> Testing
> -------
> 
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>


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