This is an automatically generated e-mail. To reply, visit:

Looks like the SHARE and UNSHARE operations are not in this patch?

include/mesos/resources.hpp (lines 424 - 430)

    See my comment at the definition.

include/mesos/resources.hpp (lines 513 - 518)

    See my comment at the definition.

include/mesos/v1/resources.hpp (lines 424 - 431)


include/mesos/v1/resources.hpp (lines 513 - 518)


src/common/resources.cpp (line 1169)

    Chatted offline but I think we do need to `clear_shared()` and 
`clear_disk()` for shared resources here. 
    The problem here is that only quantities can be aggregated across slaves 
and if `clear_shared()` and `clear_disk()` are not called, the resources have 
identities and cannot be aggregated across slaves. e.g., different slaves can 
have persistent volumes with the same size and ID but are actually different.

src/common/resources.cpp (lines 1311 - 1316)

    Chatted about removing `destroyAllowed` method but here an additional point 
is that: determining if a shared resource is no longer used and thus can be 
destroyed is the responsiblity of the master (more specifically the 
valication.cpp file in it and not the Resource abstraction. 
    Here the concern should be: if the resource is in there, destroy it, return 
Error otherwise. Therefore the original code is still correct.

src/common/resources.cpp (line 1328)

    Should we require persistent volumes to be unshared before we destroy them?
    Here the concern is that we are clearing things in DESTROY that are not 
added by its pairing operation CREATE.

src/common/resources.cpp (lines 1464 - 1496)

    This concept of weightedGet tightly couples with the sorter logic which is 
not used elsewhere so it shouldn't be pulled out.
    Here to have something that's more easily explainable in a generic way, 
perhaps we can introduce something like
    // Returns the count of the target Resource in the Resources.
    size_t Resources::count(const Resource& target) const
      // Search the list of resources in the collection.
      // For shared resources, return the Resource_::sharedCount but because 
      // is strictly an internal optimzation, the caller just needs to refer to 
      // as "the count".
      // For nonshared resources, given how we collapse addable resources, the 
      // is at most 1.
    I feel API is easiler to explain in a general sense for the callers and the 
implementation is easy to explain for readers of the cpp file without going to 
through the DRF logic.
    Chatted with Anindya offline and we are still debating whether we can avoid 
even having `count` altogether but I am writing down my thoughts here.
    As to whether we are leaking internal information, I don't feel like this 
is the case: we are not asking the caller to carefully manipulating the 
internal state in order to use share resource arithmetics correctly but rather 
this provides additional information for logic that doesn't deal with resource 
arithmetic. Therefore I don't think we are breaking abstractions but merely 
exposing a const `count` method.

src/common/resources.cpp (lines 1613 - 1635)

    If we don't need `weightedGet` we don't need this either.

src/common/resources.cpp (line 1980)

    I understand that there are cases where we call `<<` on the Resource 
directly so the `Resource_` version doesn't get called, but perhaps `<SHARED>` 
pairs better with the shared count `<6>`?

src/common/resources_utils.hpp (lines 41 - 45)

    The comments at the definitions.

src/common/resources_utils.cpp (lines 71 - 74)

    Do we need `Resources::isShared(resource)`?
    If we don't need that method, here we can actually just 

src/common/resources_utils.cpp (lines 88 - 119)

    In the most recent iteration we can just -= from the call sites right?

src/common/resources_utils.cpp (lines 122 - 139)

    This method is basically `Resources& Resources::operator+=(const Resources& 
that)` except that it doesn't increment the shared count.
    The method name doesn't suggest this so it could be confusing but moreover 
it looks like the same logic can be achieved at the call sites using `+=` but 
"not add stuff if it is already in there".
    See the comments at the call site in the allocation.

src/master/allocator/mesos/hierarchical.cpp (line 445)

    Shouldn't `Resources::sum(used)` already include shared resources?
    Also, shared resources in `total` are not all `allocated`.
    Ideally we should only have one copy of the shared resources here even 
though multiple tasks are using it. Instead of using `sum`, we can just add the 
resources one by one and check if 
`slaves[slaveId].allocated.contains(resource)` before adding it.

src/master/allocator/mesos/hierarchical.cpp (line 890)

    We can keep using `slaves[slaveId].allocated -= resources;` right?

src/master/allocator/mesos/hierarchical.cpp (lines 1267 - 1270)

    Instead of stating nonshared resources are accounted for here and attach 
shared resources later, we should explain the algorithm here.
    For shared resources if we just take it from `slaves[slaveId].total`, then 
`available` becomes:
    Resources available = (slaves[slaveId].total - 
slaves[slaveId].allocated).nonshared() + slaves[slaveId].total.shared();

src/master/allocator/mesos/hierarchical.cpp (line 1307)

    According to my comment above, shared resources can be handled together 
with nonshared resources, i.e., this line above
    Resources available = slaves[slaveId].total - slaves[slaveId].allocated;
    is sufficient.
    Plus there are a few issues if we do it here:
    1. Yes currently shared resources happen to be all reserved but it's not 
the allocator's concern here so we don't need to limit it to reserved(role) for 
futureproofness and that we don't need to explain this here.
    2. Of course we don't want to use resources reserevd for other roles but 
that's captured in a line above: `Resources resources = (available.unreserved() 
+ available.reserved(role)).nonRevocable();`
    3. We do want `allocatable` and `isFiltered` to work with the shared 
resources as well. (e.g., the framework has filtered the shared resources 
because it doesn't want it)

src/master/allocator/mesos/hierarchical.cpp (lines 1317 - 1326)

    I understand that we should only add shared resources if it is not already 
in here but we don't need to call a separate helper whose purpose is hard to 
describe out of the context here.
    We can just do this:
    // Remove the shared resources already in 'allocated' first so there's
    // no duplicates in the result.
    slaves[slaveId].allocated -= resources.shared();
    slaves[slaveId].allocated += resources;

src/master/allocator/mesos/hierarchical.cpp (lines 1402 - 1405)

    Ditto to comments on the quota stage.
    Note that here if we do
    Resources available = (slaves[slaveId].total - 
slaves[slaveId].allocated).nonshared() + slaves[slaveId].total.shared();
    The same shared resources are added to available again and could be offered 
to another framework. This is in line with the design that a different offer 
can go to a different framework in the second stage. 
    Frameworks outside of the role will not see the shared volumes so that's 
not a concern either.

src/master/allocator/mesos/hierarchical.cpp (line 1405)

    Use two space indentation to continue after a `=`.

src/master/allocator/mesos/hierarchical.cpp (lines 1470 - 1472)

    Ditto to my comments on the quota stage.

src/master/allocator/mesos/hierarchical.cpp (line 1483)

    Ditto to my comments on the quota stage.

src/master/allocator/sorter/drf/sorter.hpp (line 22)

    Not needed?

src/master/allocator/sorter/drf/sorter.cpp (lines 19 - 25)

    No longer needed?

src/master/allocator/sorter/drf/sorter.cpp (line 130)

    Similar to the allocator code:
    // Remove the shared resources already in 'allocated' first so there's
    // no duplicates in the result.
    allocations[name].resources[slaveId] -= resources.shared();
    allocations[name].resources[slaveId] += resources;

src/master/allocator/sorter/drf/sorter.cpp (lines 132 - 133)

    We should clearly document this in the comments for 
`Allocation::scalarQuantites`: it only includes nonshared resources because 
shared resources can only be aggregated during according to the total during 

src/master/allocator/sorter/drf/sorter.cpp (line 250)

    Compared to `allocate()` this is even simpler here: 
`allocations[name].resources[slaveId] -= resources;` should just work right?

src/master/allocator/sorter/drf/sorter.cpp (lines 252 - 254)

    What does "included in non shareable resources already." mean?

src/master/allocator/sorter/drf/sorter.cpp (lines 271 - 274)

    Here we should actually add shared resources into `total_.scalarQuantities` 
because `total_.scalarQuantities` doesn't change with allocations and we need 
`total_.scalarQuantities.names()` to return all names of resources in the pool.

src/master/allocator/sorter/drf/sorter.cpp (lines 290 - 296)

    Here we should actually remove shared resources from 

src/master/allocator/sorter/drf/sorter.cpp (lines 394 - 402)

    `allocations[(*it).name].scalarQuantities` has only nonshared resources.

src/master/allocator/sorter/drf/sorter.cpp (line 411)

    It's convenient here that `total_.scalarQuantities` still contains both 
shared and nonshared quantities.

src/master/allocator/sorter/drf/sorter.cpp (lines 427 - 432)

    Since `scalarQuantities` doesn't include any shared resources, this is in 
fact the allocation for nonshared resources.
    To add shared resource allocation on top of this one way discussed is to 
use `contains`, something like:
    foreachpair (const SlaveID& slaveId, 
                 const Resources& resources, 
                 allocations[name].resources) {
      foreach (const Resource& resource, resources.shared()) {
        unsigned count = 0;
        foreach (const Client& client, clients) {
          if (allocations[client.name].resources.contains(slaveId) &&
              allocations[client.name].resources[slaveId].contains(resource)) {
        CHECK(count > 0);
        allocation += resource.scalar() / count;
    It feels like it's not optimal that we need to go through all slaves here 
if very few/no slaves have shared resources on them. Alternatively if we 
maintain a separate `hashmap<SlaveID, Resources> sharedResources` in 
`DRFSorter::Allocation` and `DRFSorter::Total` and add a `count` method 
(described in another comment) in Resources we could do this:
    foreachpair (const SlaveID& slaveId, 
                 const Resources& resources, 
                 allocations[name].shareResources) {
      foreach (const Resource& resource, resources) {
        size_t count = total_.shareResources[slaveId].count(resource);
        CHECK(count > 0);
        allocation += resource.scalar() / count;
    This requires `allocated` and `unallocated` to update 
`total_.shareResources` though. What do you think?

src/master/master.hpp (lines 163 - 183)

    We don't need this in the most recent revision right?

src/master/master.cpp (line 3309)

    We should no longer need to pull this up.

src/master/master.cpp (line 3327)

    s/incase/in case/

src/master/master.cpp (lines 3334 - 3350)

    I see that you want `offeredResources` to include the most update-to-date 
shared count in it but instead of handling it specially here can we can always 
look at the usage in `slave.usedResources` explicitly wherever that information 
is actually needed.

src/master/master.cpp (lines 3789 - 3790)

    We should validate of the operation here, as 
             Option<Error> error = validation::operation::validate(
    -            operation.destroy(), slave->checkpointedResources);
    +            operation.destroy(),
    +            slave->checkpointedResources,
    +            slave->usedResources);

src/master/master.cpp (line 3806)

    Good eye :)

src/master/master.cpp (line 3807)

    This is probably due to rebasing but the slave->agent renaming is already 
happening so don't change it back.

src/master/master.cpp (lines 3909 - 3912)

    It's not intuitive that you have to -= on part of the resources and += on 
    Plus `_offeredResources` is what the master has offered to the framework 
and in this method we are subtracting what the task is using from the offered 
    Here let's keep the semantics unchanged.
    If so, then if a task is using a shared volume from an offer, it should 
still be taken out of the resources the same way as nonshared resources (i.e., 
    Note that when you call `Framework::removeOffer()` and 
`Slave::removeOffer`, shared resources operators should make sure 
`Framework::offeredResources`, `Framework::totalOfferedResources` and 
`Slave::offeredResources` are adjusted correctly for shared resources.

src/master/master.cpp (line 3918)

    `task_.resources()` should still be correct even in the presence of shared 

src/master/master.cpp (lines 3952 - 3961)

    Here I think this semantics is more straightforward:
    Return the shared resource to the allocator only when the frameowrk is no 
longer using a shared resource. i.e., 
      Resources recovered = _offeredResources.nonShared();
      foreach (const Resource& resource, _offeredResources.shared()) {
        if (!framework.totalUsedResources.contains(resource) &&
            !framework.totalOfferedResources.contains(resource) {
          recovered += resource;

src/master/master.cpp (lines 6757 - 6768)

    Handle it the same way as in `_accept()`.

src/master/master.cpp (line 6954)

    No need for the empty line.

src/master/validation.cpp (lines 825 - 827)

    This is where we should validate the destroy operation:
     Option<Error> validate(
         const Offer::Operation::Destroy& destroy,
    -    const Resources& checkpointedResources)
    +    const Resources& checkpointedResources,
    +    const Resources& usedResources)

src/master/validation.cpp (line 842)

      if (usedResources.contains(destroy.volumes())) {
        return Error("Persistent volumes busy");

- Jiang Yan Xu

On April 28, 2016, 5:16 p.m., Anindya Sinha wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45961/
> -----------------------------------------------------------
> (Updated April 28, 2016, 5:16 p.m.)
> Review request for mesos and Jiang Yan Xu.
> Bugs: MESOS-4431
>     https://issues.apache.org/jira/browse/MESOS-4431
> Repository: mesos
> Description
> -------
> Number of consumers for each shared resource is tracked. The count is
> incremented on task launch, and decremented on task termination whenever
> the task uses a shared resource.
> Allow DESTROY for shared volumes only if reference count is 0.
> Since shared resources are available to multiple frameworks of the
> same role simultaneously, we normalize it with a weight equivalent
> to the number of frameworks to which this shared resource has been
> allocated to in the sorter.
> Diffs
> -----
>   include/mesos/resources.hpp a557e97c65194d4aad879fb88d8edefd1c95b8d8 
>   include/mesos/v1/resources.hpp a5ba8fec4c9c3643646308f75a4b28cefe0b3df3 
>   src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 
>   src/common/resources_utils.hpp 2840f459288bbe8440eda08119d4f86a8be5a291 
>   src/common/resources_utils.cpp 8e881a09500b0966b577e514077460f724868a8d 
>   src/master/allocator/mesos/hierarchical.cpp 
> 0de03c7347e01fde2b42f5ec38a34a62edf642a1 
>   src/master/allocator/sorter/drf/sorter.hpp 
> 05d5205d29ad74e01e07c508d88b6f8371541513 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 4306973277b9d32356eed31ceabac09fb2a03e6c 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/master/master.cpp ff41da3d077b65b44277e1bbae88c61b7bb88a3d 
>   src/master/validation.cpp f458100d22ec1f9f10921c1c91b6931a5671e28f 
>   src/v1/resources.cpp 8c3f2d1c1529915a59d47fe37bb3fc7a3267079a 
> Diff: https://reviews.apache.org/r/45961/diff/
> Testing
> -------
> Tests successful.
> Thanks,
> Anindya Sinha

Reply via email to