----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22151/#review45151 -----------------------------------------------------------
Can you file a bug for the (existing) executor exited race condition we discussed? I commented on another (regressive) bug that is different than the one we discussed. src/common/protobuf_utils.hpp <https://reviews.apache.org/r/22151/#comment79831> Oh earlier I meant just letting the reader of the TODO know why we would make the SlaveID optional, not have the library function know about the callers. For example, if Alice comes along and reads this TODO, there's no context to guide her. src/master/hierarchical_allocator_process.hpp <https://reviews.apache.org/r/22151/#comment79834> Per your comment earlier about library functions knowing about the callers, could this comment be changed to reflect the allocator semantics instead of the master's semantics? src/master/master.hpp <https://reviews.apache.org/r/22151/#comment79838> Seems odd to say "fails" in the non failure (invalid) case, what about clarifying: // Returns None if the task is valid. // Returns a validation Error if the task is invalid. // Returns a Failure if an authorization failure occurs. src/master/master.cpp <https://reviews.apache.org/r/22151/#comment79850> newline here? src/master/master.cpp <https://reviews.apache.org/r/22151/#comment79862> Do you want the const& here? src/master/master.cpp <https://reviews.apache.org/r/22151/#comment79856> Why not just pass 'user' and have the "Not authorized ..." string inside _authorize? Seems a bit strange to pass the error string? Indentation? src/master/master.cpp <https://reviews.apache.org/r/22151/#comment79874> There is a race here where we overcommit! This is different from the race I mentioned earlier: -> executor is running -> validateTask, launchTask is now queued -> executorExited! -> launchTask unqueued, now we're adding the executor resources even though our validation did not!! - Ben Mahler On June 7, 2014, 1:37 a.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22151/ > ----------------------------------------------------------- > > (Updated June 7, 2014, 1:37 a.m.) > > > Review request for mesos, Benjamin Hindman and Ben Mahler. > > > Bugs: MESOS-1114 > https://issues.apache.org/jira/browse/MESOS-1114 > > > Repository: mesos-git > > > Description > ------- > > See summary. > > > Diffs > ----- > > src/Makefile.am 4a3f2e12643a1f02284587ebcbe9374f416b3d60 > src/common/protobuf_utils.hpp 0f653414bc1bb2b632ec8cd9c8bd7202a53d42e1 > src/master/hierarchical_allocator_process.hpp > 0c5e2e050cad96fafaf136232bd255b0ae3038cd > src/master/master.hpp e2448310aa714b5fae49b9bf4fc95859ae9d7ec3 > src/master/master.cpp 89f426c14de365369b900864f1983b1f9260953f > src/tests/master_authorization_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/22151/diff/ > > > Testing > ------- > > make check. > > Will write new tests and update this review or will create a new one. > > > Thanks, > > Vinod Kone > >
