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