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