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