----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51270/#review146410 -----------------------------------------------------------
Fix it, then Ship it! Some higher-level comments: -Should we ensure that task group executors (DEFAULT or CUSTOM) have some disk allocated to them? We're stuck not enforcing this for old executor for backwards compatibility but it seems prudent to disallow this for task group executors. -Do we want any policy on whether tasks within a group need to have cpu,mem,disk resources allocated to them? It seems for the DEFAULT executor we know that containers will need all of these? For CUSTOM I suppose it's possible for some tasks to only need some cpu or some mem or some disk. I wonder how we're going to express the policy of subcontainer isolation. If we only isolate at the level of the entire group + executor (a.k.a. "pod") then how will we add stricter isolation in the future without breaking those that rely on pod-level isolation? src/master/validation.hpp (lines 123 - 130) <https://reviews.apache.org/r/51270/#comment212832> Can you put this after the non-internal declarations, like we did for the other namespaces here? Even better would be to have the internal ones entirely separate at the bottom so it's a bit easier to scan the functions, but feel free to punt on that. src/master/validation.hpp (line 134) <https://reviews.apache.org/r/51270/#comment212874> "failed" seems to suggest TASK_FAILED, should you rephrase to suggest TASK_ERROR? src/master/validation.hpp (lines 135 - 137) <https://reviews.apache.org/r/51270/#comment212876> In general how about a newline between comment paragraphs, and reducing jaggedness for readability? ``` // Validates a task group that a framework attempts to launch // within the offered resources. Returns an optional error // which will cause the master to send TASK_ERROR status // updates for *all* the tasks in the task group back to // the framework. // // NOTE: Validation error of *any* task will cause all the // tasks in the task group to be rejected by the master. ``` I noticed that your NOTE says the same thing as the block above it..? Should we just remove the NOTE altogether? src/master/validation.cpp (lines 1001 - 1003) <https://reviews.apache.org/r/51270/#comment212877> Only document this up in the header for consistency? Also, newline between the comment paragraphs? src/master/validation.cpp (line 1047) <https://reviews.apache.org/r/51270/#comment212881> Specific to what? Being a task group? Looks like you can just copy your comment from `group::internal::validateTask`? src/master/validation.cpp (lines 1049 - 1051) <https://reviews.apache.org/r/51270/#comment212884> Maybe a comment that for TaskGroup we require the executor type to be set and not equal to UNKNOWN since it's a new path? Shouldn't there be a check for it being set as well? ``` if (!executor.has_type()) { return Error("'ExecutorInfo.type' must be set when lauching a task group"); } ``` src/master/validation.cpp (line 1053) <https://reviews.apache.org/r/51270/#comment212886> I realize now why you needed this variable in the other patch (so you can use .cpus, .mem). How about 'executorResources' since it's used below (1089) and there it's a bit hard to identify that 'resources' is the executor's resources? src/tests/master_validation_tests.cpp (lines 1489 - 1490) <https://reviews.apache.org/r/51270/#comment212866> Do you want to do some check of the error string to make sure? - Benjamin Mahler On Aug. 21, 2016, 11:29 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51270/ > ----------------------------------------------------------- > > (Updated Aug. 21, 2016, 11:29 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-6042 > https://issues.apache.org/jira/browse/MESOS-6042 > > > Repository: mesos > > > Description > ------- > > The TaskGroup is considered invalid if any of the tasks or the executor > is invalid. > > > Diffs > ----- > > src/master/validation.hpp fd00609df483930d9f749ce764830129da9e8a2c > src/master/validation.cpp ddc7ac3965c93138a38fe3c79776173c610393d6 > src/tests/master_validation_tests.cpp > ad89812a55fa56c2e13e2a683e35f777dc6341a9 > > Diff: https://reviews.apache.org/r/51270/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
