mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 57817: Offers not sent for suppressed roles as indicated in `SUBSCRIBE`.
Date Thu, 01 Jun 2017 17:41:40 GMT

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




src/common/protobuf_utils.hpp
Lines 359-361 (patched)
<https://reviews.apache.org/r/57817/#comment250001>

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



src/master/allocator/mesos/hierarchical.cpp
Lines 364-368 (patched)
<https://reviews.apache.org/r/57817/#comment249994>

    Ditto here.



src/master/allocator/mesos/hierarchical.cpp
Lines 391-395 (patched)
<https://reviews.apache.org/r/57817/#comment249993>

    Is this stale? Seems to refer to "deactivated" roles whereas the variables are "suppressed"
roles



src/master/allocator/mesos/hierarchical.cpp
Line 377 (original), 399-401 (patched)
<https://reviews.apache.org/r/57817/#comment250000>

    No need to guard this, it should be idempotent. It might end up being more readable without
the reader might be looking for why we leave something activated when we're only trying to
avoid deactivated what's already deactivated (but the if check is indirect so the reader has
to know that the role being suppressed means it it deactivated already).



src/master/allocator/mesos/hierarchical.cpp
Line 419 (original), 446-448 (patched)
<https://reviews.apache.org/r/57817/#comment249995>

    Deactivate is idempotent, no need to guard this? Ditto the thoughts on readability from
above.



src/master/allocator/mesos/hierarchical.cpp
Lines 449-450 (original), 479-481 (patched)
<https://reviews.apache.org/r/57817/#comment249998>

    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.



src/master/allocator/mesos/hierarchical.cpp
Lines 485 (patched)
<https://reviews.apache.org/r/57817/#comment249997>

    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.


- Benjamin Mahler


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