> On June 9, 2014, 11:26 p.m., Ben Mahler wrote: > > 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.
filed https://issues.apache.org/jira/browse/MESOS-1466 for the existing bug. > On June 9, 2014, 11:26 p.m., Ben Mahler wrote: > > src/master/master.hpp, line 319 > > <https://reviews.apache.org/r/22151/diff/4/?file=605180#file605180line319> > > > > 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. done. > On June 9, 2014, 11:26 p.m., Ben Mahler wrote: > > src/master/hierarchical_allocator_process.hpp, lines 562-564 > > <https://reviews.apache.org/r/22151/diff/4/?file=605179#file605179line562> > > > > 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? n/a now. > On June 9, 2014, 11:26 p.m., Ben Mahler wrote: > > src/common/protobuf_utils.hpp, line 51 > > <https://reviews.apache.org/r/22151/diff/4/?file=605178#file605178line51> > > > > 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. SlaveID can be optional because StatusUpdate.SlaveID is optional. Updated the comment. > On June 9, 2014, 11:26 p.m., Ben Mahler wrote: > > src/master/master.cpp, lines 2017-2021 > > <https://reviews.apache.org/r/22151/diff/4/?file=605181#file605181line2017> > > > > Why not just pass 'user' and have the "Not authorized ..." string > > inside _authorize? Seems a bit strange to pass the error string? > > > > Indentation? because it's used in role authorization too. > On June 9, 2014, 11:26 p.m., Ben Mahler wrote: > > src/master/master.cpp, lines 2061-2070 > > <https://reviews.apache.org/r/22151/diff/4/?file=605181#file605181line2061> > > > > 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!! good catch. refactored the code to delay resource usage checking. > On June 9, 2014, 11:26 p.m., Ben Mahler wrote: > > src/master/master.cpp, line 1984 > > <https://reviews.apache.org/r/22151/diff/4/?file=605181#file605181line1984> > > > > Do you want the const& here? n/a now. > On June 9, 2014, 11:26 p.m., Ben Mahler wrote: > > src/master/master.cpp, line 1515 > > <https://reviews.apache.org/r/22151/diff/4/?file=605181#file605181line1515> > > > > newline here? done - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22151/#review45151 ----------------------------------------------------------- On June 10, 2014, 6:27 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22151/ > ----------------------------------------------------------- > > (Updated June 10, 2014, 6:27 p.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 26af1139a43a62b91712acd158b24a8977c81d3f > src/master/master.cpp c18ccc4a1770cd68e4c3cb4b5f8ab912515ab613 > 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 > >
