> On Feb. 27, 2014, 12:26 p.m., Benjamin Hindman wrote:
> > src/tests/fault_tolerance_tests.cpp, line 665
> > <https://reviews.apache.org/r/16724/diff/7/?file=497356#file497356line665>
> >
> > Please move '{' to newline and use const & for parameters (and did you
> > really want to return a 'const string'?).
> >
> > Also, please add a healthier comment that no only should we "use use a
> > real JSON parser" but this will not return the entire value in the event
> > that the value is an object or an array since it will stop at the first
> > comma.
'{' moved to newline, parameters const&'d.
As for the return value, I've changed the function to bool
isJsonValueEmpty(text, key) to ensure that it doesn't get misused under the
assumption that it returns a full value. Added a comment in the implementation
(doesn't need to be a function comment since the function only returns bool
now).
> On Feb. 27, 2014, 12:26 p.m., Benjamin Hindman wrote:
> > src/tests/fault_tolerance_tests.cpp, line 759
> > <https://reviews.apache.org/r/16724/diff/7/?file=497356#file497356line759>
> >
> > Is this supposed to be master.get() or slave.get()? Also, there is a
> > lot of code here, can you put a newline between the masterState "stanza"
> > and the slaveState "stanza" to make it more readable?
Good catch. Thanks! Fixed all instances.
> On Feb. 27, 2014, 12:26 p.m., Benjamin Hindman wrote:
> > src/tests/fault_tolerance_tests.cpp, line 672
> > <https://reviews.apache.org/r/16724/diff/7/?file=497356#file497356line672>
> >
> > Can you leave a comment as to why you've created this here, how it
> > differs from 'createTask' in mesos.hpp and perhaps even a TODO to move this
> > into mesos.hpp? My guess is that you could easily overload 'createTask' in
> > mesos.hpp and you get this behavior or you get the 'command' behavior if
> > you pass a 'command' string.
An ExecutorInfo has a 'command' string too, but also an ExecutorID, so I've
merged createTaskWith[Default]Executor into createTask by adding an extra
"const Option<mesos::ExecutorID>& executorId = None()," parameter. If isNone(),
the we use a CommandInfo(command), otherwise we use an ExecutorInfo(executorId,
command). Now I can use "createTask(offers.get()[0], "exit 1",
DEFAULT_EXECUTOR_ID);" to get the DEFAULT_EXECUTOR_INFO behavior.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16724/#review35689
-----------------------------------------------------------
On Feb. 27, 2014, 12:28 p.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16724/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2014, 12:28 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-767
> https://issues.apache.org/jira/browse/MESOS-767
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added completed frameworks/tasks to slave re-registration.
> Fixes MESOS-767.
>
> Additional issues discovered during investigation:
> - MESOS-905: Remove Framework.id in favor of FrameworkInfo.id
> - MESOS-906: Last task in Completed Framework never graduates from
> terminatedTasks to completedTasks.
> - Completed frameworks/executors/tasks are stored in circular buffers,
> and these may overflow in different orders on different slaves.
> BenH proposes an archive to replace these circular buffers.
>
>
> Diffs
> -----
>
> include/mesos/scheduler.hpp 2e4707e
> src/master/master.hpp 7649737
> src/master/master.cpp 77872ec
> src/messages/messages.proto 922a8c4
> src/slave/slave.cpp 2d21e16
> src/tests/fault_tolerance_tests.cpp 60e06cc
> src/tests/mesos.hpp d7bdaee
>
> Diff: https://reviews.apache.org/r/16724/diff/
>
>
> Testing
> -------
>
> make check; manually failed-over a master, watched the slave reregister its
> completed frameworks, web UI shows completed tasks and stdout/stderr.
> Added a new unit/integration test to verify the expected behavior.
>
>
> Thanks,
>
> Adam B
>
>
