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 57817: Offers not sent for suppressed roles as indicated in `SUBSCRIBE`.
Date Fri, 02 Jun 2017 17:49:30 GMT


> On June 1, 2017, 5:41 p.m., Benjamin Mahler wrote:
> > src/common/protobuf_utils.hpp
> > Lines 359-361 (patched)
> > <https://reviews.apache.org/r/57817/diff/11/?file=1735873#file1735873line359>
> >
> >     This looks to be just a generic conversion from repeated ptr field to set, not
something role specific?
> >     
> >     This means we should either inline the set construction (using the iterator
approach), or expose a generic conversion (would we want to return an Error if there are duplicates?)

Agreed. Removed the function and calling it inline instead.


> On June 1, 2017, 5:41 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 391-395 (patched)
> > <https://reviews.apache.org/r/57817/diff/11/?file=1735877#file1735877line391>
> >
> >     Is this stale? Seems to refer to "deactivated" roles whereas the variables are
"suppressed" roles

Since `deactivate()` is idempotent (as in next comment), there is no need to add the `if (!framework.suppressedRoles.count(role))`
condition, and hence I do not think the comment is needed here.


> On June 1, 2017, 5:41 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 449-450 (original), 479-481 (patched)
> > <https://reviews.apache.org/r/57817/diff/11/?file=1735877#file1735877line479>
> >
> >     Not your bug, but it seems wrong to be activating roles here if the framework
was deactivated. Would be good to sync with mpark on this original change. It seems to me
that if the framework is deactivated via deactivateFramework, we wouldn't want to be activating
things here in updateFrameowrk.

Reached out to @mpark. I kept it as-is till he has a chance to look into it.


> On June 1, 2017, 5:41 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 485 (patched)
> > <https://reviews.apache.org/r/57817/diff/11/?file=1735877#file1735877line485>
> >
> >     Seems there is a bug here? What about existing roles (not covered by "removed
roles" or "added roles" loops above) that have now become suppressed? For these, we want to
deactivate them.

Yes, that is correct.
So, the roles that need to be deactivated are the roles which have been removed, as well as
the roles which have moved from non-suppressed mode to suppressed mode. Similarly, the roles
that need to be activated are the roles that have been added, as well as the roles which have
moved from suppressed to non-suppressed mode.


- Anindya


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


On June 1, 2017, 12:38 a.m., Anindya Sinha wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57817/
> -----------------------------------------------------------
> 
> (Updated June 1, 2017, 12:38 a.m.)
> 
> 
> Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7015
>     https://issues.apache.org/jira/browse/MESOS-7015
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> If the `SUBSCRIBE` indicates a subset of roles to be suppressed during
> framework (re)registration, the allocator does not offer resources for
> those roles to such frameworks. Note that this functionality is added
> for `v1::SUBSCRIBE` only (and not for scheduler driver API).
> 
> In addition, the master validates the suppressed roles to be a subset
> of all the roles being (re)registered by the framework.
> 
> 
> Diffs
> -----
> 
>   include/mesos/allocator/allocator.hpp b31fbcfc18cbbe1e6b5fb5ccbc234f790326a5b4 
>   src/common/protobuf_utils.hpp be2325f05b81b847fa592eff65175cbc99764fd6 
>   src/common/protobuf_utils.cpp 3fcaf786b29a00f003c10b0f1614a2c7eddc725d 
>   src/master/allocator/mesos/allocator.hpp b2dcb566c49a1bbb1d955ca46e1c8eef91e62733 
>   src/master/allocator/mesos/hierarchical.hpp 5e7c3068061012c51d4b9220dedf476408016a12

>   src/master/allocator/mesos/hierarchical.cpp 6679d47ea53bbcd68d375654edf6e85890e5772a

>   src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
>   src/master/master.cpp c66907bd55cb2eb549ec89f048d41376df556eb9 
>   src/tests/allocator.hpp 0b9fdeaccc65bb6988103ac156e0584cf5df17fe 
>   src/tests/hierarchical_allocator_tests.cpp eb2b647d3247a85e8b9b82e5589232c74ad8570f

>   src/tests/master_allocator_tests.cpp 0082045ccaebe015725c5e178aa89dac32b1c50c 
>   src/tests/persistent_volume_endpoints_tests.cpp 9b820b8f4e0d0e2c62b5c9b420b790cdfb59dd0c

>   src/tests/reservation_tests.cpp 34628eed671aee7ed69497d282c8043d65620c14 
>   src/tests/resource_offers_tests.cpp 7ad037e5ec1d63cc8c2b8b0ad092e3f5f6f402c8 
>   src/tests/slave_recovery_tests.cpp e140f4d902a43b8fb2390541a357a217896b6228 
> 
> 
> Diff: https://reviews.apache.org/r/57817/diff/12/
> 
> 
> Testing
> -------
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>


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