----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22151/#review45184 -----------------------------------------------------------
Ship it! src/tests/master_authorization_tests.cpp <https://reviews.apache.org/r/22151/#comment79909> Looks like a copy paste with a lot of unneeded stuff? src/common/protobuf_utils.hpp <https://reviews.apache.org/r/22151/#comment80088> Just curious, isn't this trivial to add? All callers would still work? src/master/master.hpp <https://reviews.apache.org/r/22151/#comment80049> Looks like this comment needs an update w.r.t. the return value. src/master/master.hpp <https://reviews.apache.org/r/22151/#comment80074> how about calling this 'validationError'? src/master/master.cpp <https://reviews.apache.org/r/22151/#comment80054> :) src/master/master.cpp <https://reviews.apache.org/r/22151/#comment80057> Maybe a little note here that that tasks are added to 'pendingTasks' after being validated and that's why this works? (Non-local reasoning is needed here to know why this 'pendingTasks' lookup works). src/master/master.cpp <https://reviews.apache.org/r/22151/#comment80059> because validators use 'pendingTasks' to determine that the executorInfos are all the same? src/master/master.cpp <https://reviews.apache.org/r/22151/#comment80076> There was a bit more reasoning to it than this, right? Using one continuation made it easier to prevent races with exited executors and it's less code too! src/master/master.cpp <https://reviews.apache.org/r/22151/#comment80060> Maybe a TODO to: Not use the heap, make the visit operation const, use a vector, and use a fixed visitor list that gets created during initialization? src/master/master.cpp <https://reviews.apache.org/r/22151/#comment80079> 'validationErrors'? src/master/master.cpp <https://reviews.apache.org/r/22151/#comment80080> Should this message be prefixed differently for both cases? "Authorization failed: ..." and "Invalid task: ..." (includes not authorized case) This should help the framework developers understand the messages coming out. src/master/master.cpp <https://reviews.apache.org/r/22151/#comment80086> Maybe a little note that launchTask below will add the executor? (non-local reasoning needed) - Ben Mahler 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 > >
