> On June 10, 2014, 10:10 p.m., Ben Mahler wrote: > > src/common/protobuf_utils.hpp, lines 51-52 > > <https://reviews.apache.org/r/22151/diff/5/?file=606524#file606524line51> > > > > Just curious, isn't this trivial to add? All callers would still work?
I wanted to make it optional and have a default of None() which means it needs to be at the end of non-optional arguments, which is a signature change. > On June 10, 2014, 10:10 p.m., Ben Mahler wrote: > > src/master/master.hpp, lines 326-327 > > <https://reviews.apache.org/r/22151/diff/5/?file=606526#file606526line326> > > > > Looks like this comment needs an update w.r.t. the return value. done > On June 10, 2014, 10:10 p.m., Ben Mahler wrote: > > src/master/master.hpp, line 337 > > <https://reviews.apache.org/r/22151/diff/5/?file=606526#file606526line337> > > > > how about calling this 'validationError'? called it validationErrors in the .cpp. Kept "f" here to avoid wrapping. > On June 10, 2014, 10:10 p.m., Ben Mahler wrote: > > src/master/master.cpp, lines 1563-1566 > > <https://reviews.apache.org/r/22151/diff/5/?file=606527#file606527line1563> > > > > 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). done > On June 10, 2014, 10:10 p.m., Ben Mahler wrote: > > src/master/master.cpp, lines 1888-1889 > > <https://reviews.apache.org/r/22151/diff/5/?file=606527#file606527line1888> > > > > because validators use 'pendingTasks' to determine that the > > executorInfos are all the same? yes and also used by UniqueTaskID checker. > On June 10, 2014, 10:10 p.m., Ben Mahler wrote: > > src/master/master.cpp, lines 1894-1895 > > <https://reviews.apache.org/r/22151/diff/5/?file=606527#file606527line1894> > > > > 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! this was the necessary reason. what you mention is good to have reason :) > On June 10, 2014, 10:10 p.m., Ben Mahler wrote: > > src/master/master.cpp, line 2067 > > <https://reviews.apache.org/r/22151/diff/5/?file=606527#file606527line2067> > > > > 'validationErrors'? done > On June 10, 2014, 10:10 p.m., Ben Mahler wrote: > > src/master/master.cpp, lines 2142-2143 > > <https://reviews.apache.org/r/22151/diff/5/?file=606527#file606527line2142> > > > > Maybe a little note that launchTask below will add the executor? > > (non-local reasoning needed) done > On June 10, 2014, 10:10 p.m., Ben Mahler wrote: > > src/master/master.cpp, lines 1929-1935 > > <https://reviews.apache.org/r/22151/diff/5/?file=606527#file606527line1929> > > > > 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? done - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22151/#review45184 ----------------------------------------------------------- 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 > >
