----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72738/#review221547 -----------------------------------------------------------
Ship it! Overall structure looks good! I just left a documentation suggestion, but giving a ship it anyway and happy to take another look once updated if you like. include/mesos/scheduler/scheduler.proto Lines 250 (patched) <https://reviews.apache.org/r/72738/#comment310611> Hm.. seems fine to me, but I do wonder if this is the best name vs say.. FirstClassAttribute include/mesos/scheduler/scheduler.proto Lines 264-270 (patched) <https://reviews.apache.org/r/72738/#comment310612> I guess we can put the braces on the same line for these? Seems a bit cleaner include/mesos/scheduler/scheduler.proto Lines 273-274 (patched) <https://reviews.apache.org/r/72738/#comment310613> Is there a TODO here to show intent of more being added? (like you have for resources below) include/mesos/scheduler/scheduler.proto Lines 282 (patched) <https://reviews.apache.org/r/72738/#comment310605> Here's a suggestion: ``` Frameworks can express offer constraints for their roles. Constraints restrict which offers will be sent to the framework: if an offer does not match the provided constraints, it will not be sent to the framework. Constraints are expressed on a per role basis. If you consider a scheduler that has multiple apps to launch within a single role, the structure of the constraints for that role looks as follows: app 1 app 2 app n constraints OR constraints OR ... OR constraints /\ / \ constraint 1 AND constraint 2 AND ... AND constraint n That is, one of the constraint groups must match for an offer to be generated, and within a group all the constraints must match. As a concrete example, consider a scheduler with two applications with multiple instances it wants to launch within a role: application 1: hostname == "foo" application 2: unique "rack" attribute Assuming there are already some instances of application 2 launched, the constraints might look like the following: app 1 app 2 constraints OR constraints /\ /\ / \ / \ hostname == "foo" rack != "X" AND rack != "Y" AND rack != "Z" == (hostname == "foo") OR (rack != "X" AND rack != "Y" AND rack != "Z") The benefits of expressing constraints are: (1) reduced amount of unusable offers received, and hence: (2) reduced traffic and processing overhead due to unusable offer / DECLINE back and forth churn (3) most importantly, reduced latency to receive the desired offer for a particular task ``` Hope that eliminates the need for the lower level comments on RoleConstraints and Group. Would be good to get some another pair of eyes on this documentation to make sure people can understand it clearly. This would also translate pretty cleanly to our website docs? src/internal/devolve.cpp Lines 248-249 (original), 248-249 (patched) <https://reviews.apache.org/r/72738/#comment310610> Just to double check, I assume you found this by searching for suppressed_roles or some other piece of the message? - Benjamin Mahler On Aug. 6, 2020, 4:33 p.m., Andrei Sekretenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72738/ > ----------------------------------------------------------- > > (Updated Aug. 6, 2020, 4:33 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-10171 > https://issues.apache.org/jira/browse/MESOS-10171 > > > Repository: mesos > > > Description > ------- > > This patch adds framework's offer constraints into `Subscribe` and > `UpdateFramework` calls, which is a prerequisite to implementing > constraints-based offer filtering (see MESOS-10161). > > > Diffs > ----- > > include/mesos/scheduler/scheduler.proto > 6e1639a9baf017fa87b274daeedc821389216ddc > include/mesos/v1/scheduler/scheduler.proto > eb5fdeb984b28403bd8281742bd0a5f2053863e3 > src/internal/devolve.cpp 4527c522473b74622055e1765740e3706b95afdb > > > Diff: https://reviews.apache.org/r/72738/diff/1/ > > > Testing > ------- > > > Thanks, > > Andrei Sekretenko > >
