----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30459/#review70672 -----------------------------------------------------------
Ship it! src/master/master.hpp <https://reviews.apache.org/r/30459/#comment116017> A NOTE here would be nice as well. src/master/validation.hpp <https://reviews.apache.org/r/30459/#comment116011> s/give/given/ Who is using this? As it stands this comment could just be deleted, is there anything you want to say about this? E.g. ``` // Validates resources specified by XXX. // NOTE: We cannot take 'Resources' here because invalid resources are // silently ignored within its constructor. ``` src/master/validation.hpp <https://reviews.apache.org/r/30459/#comment116014> s/within the resources provided by offers/within the offered resources/ Can you add a NOTE that this must be called sequentially for each task, each task needs to be launched before the next can be validated, right? src/master/validation.hpp <https://reviews.apache.org/r/30459/#comment116006> Let's leave a bit NOTE here about why we need these! src/master/validation.cpp <https://reviews.apache.org/r/30459/#comment116020> I'm a bit confused by this comment, isn't it the case that all validations are performed regardless of whether the task is launched..? Not following why it's called out specifically for this validation. What about: ``` Validates that the TaskID does not collide with any existing tasks for the framework. ``` src/master/validation.cpp <https://reviews.apache.org/r/30459/#comment116024> Not yours, but can you add a TODO here to remove this in favor of a CHECK, because the allocator should filter these out! src/master/validation.cpp <https://reviews.apache.org/r/30459/#comment116029> Could this just be: ``` if (slaveId.isNone()) { slaveId = slave->id; } if (slave->id != slaveId.get()) { return Error(...); } ``` The existing message is a bit confusing, but let's punt for now. Mind adding the != operator if it's not there? src/master/validation.cpp <https://reviews.apache.org/r/30459/#comment116030> Why not use the `bind` approach here as well? - Ben Mahler On Jan. 31, 2015, 2:09 a.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30459/ > ----------------------------------------------------------- > > (Updated Jan. 31, 2015, 2:09 a.m.) > > > Review request for mesos, Ben Mahler, Dominic Hamon, and Vinod Kone. > > > Bugs: MESOS-2305 > https://issues.apache.org/jira/browse/MESOS-2305 > > > Repository: mesos-incubating > > > Description > ------- > > Refactored task/offer/resource valiation in master. > > See ticket for motivation. > > > Diffs > ----- > > src/Makefile.am 07bea1fb8f0035413f2119859e16fa4f9383f68e > src/master/master.hpp 337e00aa46ea127f3667e3383d631c3fb8e22f30 > src/master/master.cpp 10056861b95ed9453c971787982db7d09f09f323 > src/master/validation.hpp PRE-CREATION > src/master/validation.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/30459/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jie Yu > >
