You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Vinod Kone <vi...@gmail.com> on 2012/08/30 01:09:25 UTC
Review Request: Slave Restart (Part Uno): Factoring out Slave State
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6842/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Ben Mahler.
Description
-------
And so it begins...
I am discarding the old review associated with this (previously called path: https://reviews.apache.org/r/4944/), in favor of a fresh start.
slave/state.{hpp,cpp} contains all the functions that deal with persistent state of the slave.
This addresses bug MESOS-110.
https://issues.apache.org/jira/browse/MESOS-110
Diffs
-----
src/Makefile.am aaceee3
src/common/protobuf_utils.hpp PRE-CREATION
src/slave/slave.hpp 10c537b
src/slave/slave.cpp 4efd41e
src/slave/state.hpp PRE-CREATION
src/slave/state.cpp PRE-CREATION
src/tests/slave_state_tests.cpp PRE-CREATION
third_party/libprocess/include/stout/format.hpp c746ad3
third_party/libprocess/include/stout/os.hpp b1eceb3
Diff: https://reviews.apache.org/r/6842/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Slave Restart (Part Uno): Factoring out Slave State
Posted by Vinod Kone <vi...@gmail.com>.
> On Sept. 1, 2012, 12:19 a.m., Ben Mahler wrote:
> > src/common/protobuf_utils.hpp, line 37
> > <https://reviews.apache.org/r/6842/diff/1/?file=147306#file147306line37>
> >
> > just directly return?
> >
> > return (state == TASK_FINISHED ||
> > state == TASK_FAILED ||
> > state == TASK_KILLED ||
> > state == TASK_LOST);
> >
> >
done
> On Sept. 1, 2012, 12:19 a.m., Ben Mahler wrote:
> > src/common/protobuf_utils.hpp, line 59
> > <https://reviews.apache.org/r/6842/diff/1/?file=147306#file147306line59>
> >
> > why not do:
> >
> > if (executorId.value() != "")
unfortunately, != operators are not defined for our protobufs. we should probably fix that in another review.
> On Sept. 1, 2012, 12:19 a.m., Ben Mahler wrote:
> > src/slave/state.hpp, line 41
> > <https://reviews.apache.org/r/6842/diff/1/?file=147309#file147309line41>
> >
> > 1. perhaps we can differentiate between files and dirs here?
> >
> > like:
> > SLAVEID_FILEPATH
> > SLAVE_PATH
> >
> > 2. This makes it a bit tricky to see the required formatting, what if we just made helpers instead that takes the necessary arguments that internally knows how to use these with strings::format
1) I think PATH is generic for both dirs and files. I'm not convinced having different names is better.
2) done
> On Sept. 1, 2012, 12:19 a.m., Ben Mahler wrote:
> > src/slave/state.cpp, line 62
> > <https://reviews.apache.org/r/6842/diff/1/?file=147310#file147310line62>
> >
> > does this also apply for the empty list?
done
> On Sept. 1, 2012, 12:19 a.m., Ben Mahler wrote:
> > src/slave/state.cpp, line 77
> > <https://reviews.apache.org/r/6842/diff/1/?file=147310#file147310line77>
> >
> > would we normally just use a pointer for this?
i would like to avoid pointers as much as possible :)
> On Sept. 1, 2012, 12:19 a.m., Ben Mahler wrote:
> > src/slave/state.cpp, line 89
> > <https://reviews.apache.org/r/6842/diff/1/?file=147310#file147310line89>
> >
> > empty list here as well?
done
> On Sept. 1, 2012, 12:19 a.m., Ben Mahler wrote:
> > src/slave/state.cpp, line 180
> > <https://reviews.apache.org/r/6842/diff/1/?file=147310#file147310line180>
> >
> > I'll be fixing some of these os command to be try<nothing> since we never return false, might be best to still check for false here for now
i don't see the advantage?
> On Sept. 1, 2012, 12:19 a.m., Ben Mahler wrote:
> > src/slave/state.cpp, line 220
> > <https://reviews.apache.org/r/6842/diff/1/?file=147310#file147310line220>
> >
> > maybe check for false as well, even though it'll never happen
ditto
> On Sept. 1, 2012, 12:19 a.m., Ben Mahler wrote:
> > src/slave/state.cpp, line 222
> > <https://reviews.apache.org/r/6842/diff/1/?file=147310#file147310line222>
> >
> > formatting
looks ok to me. bad, RB formatting?
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6842/#review10959
-----------------------------------------------------------
On Aug. 29, 2012, 11:09 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6842/
> -----------------------------------------------------------
>
> (Updated Aug. 29, 2012, 11:09 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> And so it begins...
>
> I am discarding the old review associated with this (previously called path: https://reviews.apache.org/r/4944/), in favor of a fresh start.
>
> slave/state.{hpp,cpp} contains all the functions that deal with persistent state of the slave.
>
>
> This addresses bug MESOS-110.
> https://issues.apache.org/jira/browse/MESOS-110
>
>
> Diffs
> -----
>
> src/Makefile.am aaceee3
> src/common/protobuf_utils.hpp PRE-CREATION
> src/slave/slave.hpp 10c537b
> src/slave/slave.cpp 4efd41e
> src/slave/state.hpp PRE-CREATION
> src/slave/state.cpp PRE-CREATION
> src/tests/slave_state_tests.cpp PRE-CREATION
> third_party/libprocess/include/stout/format.hpp c746ad3
> third_party/libprocess/include/stout/os.hpp b1eceb3
>
> Diff: https://reviews.apache.org/r/6842/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Uno): Factoring out Slave State
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6842/#review10959
-----------------------------------------------------------
partial review before heading out for the day
src/common/protobuf_utils.hpp
<https://reviews.apache.org/r/6842/#comment23529>
just directly return?
return (state == TASK_FINISHED ||
state == TASK_FAILED ||
state == TASK_KILLED ||
state == TASK_LOST);
src/common/protobuf_utils.hpp
<https://reviews.apache.org/r/6842/#comment23530>
why not do:
if (executorId.value() != "")
src/slave/state.hpp
<https://reviews.apache.org/r/6842/#comment23531>
1. perhaps we can differentiate between files and dirs here?
like:
SLAVEID_FILEPATH
SLAVE_PATH
2. This makes it a bit tricky to see the required formatting, what if we just made helpers instead that takes the necessary arguments that internally knows how to use these with strings::format
src/slave/state.cpp
<https://reviews.apache.org/r/6842/#comment23532>
again, it would be great to wrap these format's up into helpers, seems prone to making a mistake this way
src/slave/state.cpp
<https://reviews.apache.org/r/6842/#comment23533>
does this also apply for the empty list?
src/slave/state.cpp
<https://reviews.apache.org/r/6842/#comment23535>
would we normally just use a pointer for this?
src/slave/state.cpp
<https://reviews.apache.org/r/6842/#comment23536>
empty list here as well?
src/slave/state.cpp
<https://reviews.apache.org/r/6842/#comment23537>
I'll be fixing some of these os command to be try<nothing> since we never return false, might be best to still check for false here for now
src/slave/state.cpp
<https://reviews.apache.org/r/6842/#comment23538>
maybe check for false as well, even though it'll never happen
src/slave/state.cpp
<https://reviews.apache.org/r/6842/#comment23539>
formatting
- Ben Mahler
On Aug. 29, 2012, 11:09 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6842/
> -----------------------------------------------------------
>
> (Updated Aug. 29, 2012, 11:09 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> And so it begins...
>
> I am discarding the old review associated with this (previously called path: https://reviews.apache.org/r/4944/), in favor of a fresh start.
>
> slave/state.{hpp,cpp} contains all the functions that deal with persistent state of the slave.
>
>
> This addresses bug MESOS-110.
> https://issues.apache.org/jira/browse/MESOS-110
>
>
> Diffs
> -----
>
> src/Makefile.am aaceee3
> src/common/protobuf_utils.hpp PRE-CREATION
> src/slave/slave.hpp 10c537b
> src/slave/slave.cpp 4efd41e
> src/slave/state.hpp PRE-CREATION
> src/slave/state.cpp PRE-CREATION
> src/tests/slave_state_tests.cpp PRE-CREATION
> third_party/libprocess/include/stout/format.hpp c746ad3
> third_party/libprocess/include/stout/os.hpp b1eceb3
>
> Diff: https://reviews.apache.org/r/6842/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Uno): Factoring out Slave State
Posted by Vinod Kone <vi...@gmail.com>.
> On Sept. 11, 2012, 7 p.m., Ben Mahler wrote:
> > src/Makefile.am, line 157
> > <https://reviews.apache.org/r/6842/diff/1/?file=147305#file147305line157>
> >
> > this breaks the line alignment, put it after slave/gc.cpp?
fixed tab alignment
> On Sept. 11, 2012, 7 p.m., Ben Mahler wrote:
> > src/Makefile.am, line 210
> > <https://reviews.apache.org/r/6842/diff/1/?file=147305#file147305line210>
> >
> > This also doesn't align, looks like someone used tabs for the end of line \ alignment, where tabs were treated as... 8 spaces!
> >
> > I don't see why we didn't just do 1 file per line in our makefiles.. seems harder for humans to read this way
> >
> > anyway, feel free to ignore this :)
fixed with tabs=8 spaces, for now
> On Sept. 11, 2012, 7 p.m., Ben Mahler wrote:
> > src/Makefile.am, line 769
> > <https://reviews.apache.org/r/6842/diff/1/?file=147305#file147305line769>
> >
> > ditto on the alignment
fixed
> On Sept. 11, 2012, 7 p.m., Ben Mahler wrote:
> > third_party/libprocess/include/stout/format.hpp, line 234
> > <https://reviews.apache.org/r/6842/diff/1/?file=147312#file147312line234>
> >
> > what happened here?
probably bad diff. can't see it in my file anymore.
> On Sept. 11, 2012, 7 p.m., Ben Mahler wrote:
> > third_party/libprocess/include/stout/os.hpp, line 178
> > <https://reviews.apache.org/r/6842/diff/1/?file=147313#file147313line178>
> >
> > instead of const_cast, could we just use strcpy into a non-const char* instead?
this is how we do it everywhere else in this file, so i would like to keep it this way. any particular reason you want to change it?
> On Sept. 11, 2012, 7 p.m., Ben Mahler wrote:
> > third_party/libprocess/include/stout/os.hpp, line 378
> > <https://reviews.apache.org/r/6842/diff/1/?file=147313#file147313line378>
> >
> > ditto
ditto
> On Sept. 11, 2012, 7 p.m., Ben Mahler wrote:
> > third_party/libprocess/include/stout/format.hpp, line 291
> > <https://reviews.apache.org/r/6842/diff/1/?file=147312#file147312line291>
> >
> > one idea:
> >
> > store t as a const&
> > then in get(), do the stringify(t).c_str()
i wish that worked, but it didnt :/
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6842/#review11341
-----------------------------------------------------------
On Aug. 29, 2012, 11:09 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6842/
> -----------------------------------------------------------
>
> (Updated Aug. 29, 2012, 11:09 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> And so it begins...
>
> I am discarding the old review associated with this (previously called path: https://reviews.apache.org/r/4944/), in favor of a fresh start.
>
> slave/state.{hpp,cpp} contains all the functions that deal with persistent state of the slave.
>
>
> This addresses bug MESOS-110.
> https://issues.apache.org/jira/browse/MESOS-110
>
>
> Diffs
> -----
>
> src/Makefile.am aaceee3
> src/common/protobuf_utils.hpp PRE-CREATION
> src/slave/slave.hpp 10c537b
> src/slave/slave.cpp 4efd41e
> src/slave/state.hpp PRE-CREATION
> src/slave/state.cpp PRE-CREATION
> src/tests/slave_state_tests.cpp PRE-CREATION
> third_party/libprocess/include/stout/format.hpp c746ad3
> third_party/libprocess/include/stout/os.hpp b1eceb3
>
> Diff: https://reviews.apache.org/r/6842/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Uno): Factoring out Slave State
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6842/#review11341
-----------------------------------------------------------
src/Makefile.am
<https://reviews.apache.org/r/6842/#comment24377>
this breaks the line alignment, put it after slave/gc.cpp?
src/Makefile.am
<https://reviews.apache.org/r/6842/#comment24378>
This also doesn't align, looks like someone used tabs for the end of line \ alignment, where tabs were treated as... 8 spaces!
I don't see why we didn't just do 1 file per line in our makefiles.. seems harder for humans to read this way
anyway, feel free to ignore this :)
src/Makefile.am
<https://reviews.apache.org/r/6842/#comment24379>
ditto on the alignment
third_party/libprocess/include/stout/format.hpp
<https://reviews.apache.org/r/6842/#comment24388>
what happened here?
third_party/libprocess/include/stout/format.hpp
<https://reviews.apache.org/r/6842/#comment24387>
one idea:
store t as a const&
then in get(), do the stringify(t).c_str()
third_party/libprocess/include/stout/os.hpp
<https://reviews.apache.org/r/6842/#comment24386>
comment on what this and mkdtemp returns
third_party/libprocess/include/stout/os.hpp
<https://reviews.apache.org/r/6842/#comment24383>
instead of const_cast, could we just use strcpy into a non-const char* instead?
third_party/libprocess/include/stout/os.hpp
<https://reviews.apache.org/r/6842/#comment24384>
ditto
third_party/libprocess/include/stout/os.hpp
<https://reviews.apache.org/r/6842/#comment24380>
add a comment about returning Result::none on no match
third_party/libprocess/include/stout/os.hpp
<https://reviews.apache.org/r/6842/#comment24381>
++i :)
- Ben Mahler
On Aug. 29, 2012, 11:09 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6842/
> -----------------------------------------------------------
>
> (Updated Aug. 29, 2012, 11:09 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> And so it begins...
>
> I am discarding the old review associated with this (previously called path: https://reviews.apache.org/r/4944/), in favor of a fresh start.
>
> slave/state.{hpp,cpp} contains all the functions that deal with persistent state of the slave.
>
>
> This addresses bug MESOS-110.
> https://issues.apache.org/jira/browse/MESOS-110
>
>
> Diffs
> -----
>
> src/Makefile.am aaceee3
> src/common/protobuf_utils.hpp PRE-CREATION
> src/slave/slave.hpp 10c537b
> src/slave/slave.cpp 4efd41e
> src/slave/state.hpp PRE-CREATION
> src/slave/state.cpp PRE-CREATION
> src/tests/slave_state_tests.cpp PRE-CREATION
> third_party/libprocess/include/stout/format.hpp c746ad3
> third_party/libprocess/include/stout/os.hpp b1eceb3
>
> Diff: https://reviews.apache.org/r/6842/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Uno): Factoring out Slave State
Posted by Vinod Kone <vi...@gmail.com>.
> On Sept. 13, 2012, 7:09 p.m., Benjamin Hindman wrote:
> > src/slave/state.cpp, line 34
> > <https://reviews.apache.org/r/6842/diff/2/?file=153053#file153053line34>
> >
> > In the end, I think I prefer just calling strings::format instead of get*Path. Reasoning:
> >
> > (1) You have to do it that way when you want to glob anyway, so it's kind of nice to not have two different ways to do it.
> >
> > (2) The real differences in characters are: 'get*Path' => 'strings::format(*_PATH,' and also the .get() at the end, but the .get() will go away and the rest is not so different.
> >
> > Thoughts?
That was the original intention, but I agree with BenM that having helpers would be useful for users of this interface. Because, that would obviate the need for them to remember/understand the path semantics. For eg: they dont have to know that the 1st argument should be string template, 2nd should SlaveID etc.
As I mentioned in BenM's review, I would prefer to also have helpers for glob. But I'm punting on that for C++11.
Makes sense?
> On Sept. 13, 2012, 7:09 p.m., Benjamin Hindman wrote:
> > src/slave/state.cpp, lines 168-171
> > <https://reviews.apache.org/r/6842/diff/2/?file=153053#file153053line168>
> >
> > I think for all of these you could do:
> >
> > foreach (const string& path, frameworks.get()) {
> > FrameworkID frameworkId;
> > frameworkId.set_value(os::basename(path));
> > }
> >
> > So, kill the ePath, extra const string&, etc.
done
> On Sept. 13, 2012, 7:09 p.m., Benjamin Hindman wrote:
> > src/slave/state.cpp, lines 210-211
> > <https://reviews.apache.org/r/6842/diff/2/?file=153053#file153053line210>
> >
> > I know, I know, it seems crazy, but I'd prefer:
> >
> > state.frameworks[frameworkId].executors[executorId].latest =
> > std::max(run, state.frameworks[frameworkId].executors[executorId].latest);
done, at the cost of 1 character over the limit abuse.
> On Sept. 13, 2012, 7:09 p.m., Benjamin Hindman wrote:
> > src/slave/state.cpp, line 232
> > <https://reviews.apache.org/r/6842/diff/2/?file=153053#file153053line232>
> >
> > state.frameworks[frameworkId].executors[executorId].runs[run].tasks.insert(taskId);
done
> On Sept. 13, 2012, 7:09 p.m., Benjamin Hindman wrote:
> > src/tests/slave_state_tests.cpp, line 51
> > <https://reviews.apache.org/r/6842/diff/2/?file=153054#file153054line51>
> >
> > When is this possible? Seems like you'd want to do a CHECK here.
it was needed because of the way i initialized it, fixed.
> On Sept. 13, 2012, 7:09 p.m., Benjamin Hindman wrote:
> > src/tests/slave_state_tests.cpp, lines 41-45
> > <https://reviews.apache.org/r/6842/diff/2/?file=153054#file153054line41>
> >
> > Can these guys be const? Set in constructor?
I'm not sure how to initialize protobuf objects inside an initializer list?
> On Sept. 13, 2012, 7:09 p.m., Benjamin Hindman wrote:
> > src/tests/slave_state_tests.cpp, line 58
> > <https://reviews.apache.org/r/6842/diff/2/?file=153054#file153054line58>
> >
> > I smell 'Option<string> rootDir;' instead of checking for empty string (more explicit). ;)
n/a, with the latest refactor.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6842/#review11489
-----------------------------------------------------------
On Sept. 12, 2012, 10:23 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6842/
> -----------------------------------------------------------
>
> (Updated Sept. 12, 2012, 10:23 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> And so it begins...
>
> I am discarding the old review associated with this (previously called path: https://reviews.apache.org/r/4944/), in favor of a fresh start.
>
> slave/state.{hpp,cpp} contains all the functions that deal with persistent state of the slave.
>
>
> This addresses bug MESOS-110.
> https://issues.apache.org/jira/browse/MESOS-110
>
>
> Diffs
> -----
>
> src/Makefile.am b73a024
> src/common/protobuf_utils.hpp PRE-CREATION
> src/slave/slave.hpp 7fbae8a
> src/slave/slave.cpp 4ea1db1
> src/slave/state.hpp PRE-CREATION
> src/slave/state.cpp PRE-CREATION
> src/tests/slave_state_tests.cpp PRE-CREATION
> third_party/libprocess/include/stout/format.hpp c746ad3
> third_party/libprocess/include/stout/os.hpp d08b0cb
>
> Diff: https://reviews.apache.org/r/6842/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Uno): Factoring out Slave State
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6842/#review11489
-----------------------------------------------------------
src/slave/state.cpp
<https://reviews.apache.org/r/6842/#comment24621>
In the end, I think I prefer just calling strings::format instead of get*Path. Reasoning:
(1) You have to do it that way when you want to glob anyway, so it's kind of nice to not have two different ways to do it.
(2) The real differences in characters are: 'get*Path' => 'strings::format(*_PATH,' and also the .get() at the end, but the .get() will go away and the rest is not so different.
Thoughts?
src/slave/state.cpp
<https://reviews.apache.org/r/6842/#comment24611>
I think for all of these you could do:
foreach (const string& path, frameworks.get()) {
FrameworkID frameworkId;
frameworkId.set_value(os::basename(path));
}
So, kill the ePath, extra const string&, etc.
src/slave/state.cpp
<https://reviews.apache.org/r/6842/#comment24614>
I know, I know, it seems crazy, but I'd prefer:
state.frameworks[frameworkId].executors[executorId].latest =
std::max(run, state.frameworks[frameworkId].executors[executorId].latest);
src/slave/state.cpp
<https://reviews.apache.org/r/6842/#comment24615>
Kill this and embed (see comment below).
src/slave/state.cpp
<https://reviews.apache.org/r/6842/#comment24608>
state.frameworks[frameworkId].executors[executorId].runs[run].tasks.insert(taskId);
src/tests/slave_state_tests.cpp
<https://reviews.apache.org/r/6842/#comment24604>
Can these guys be const? Set in constructor?
src/tests/slave_state_tests.cpp
<https://reviews.apache.org/r/6842/#comment24605>
When is this possible? Seems like you'd want to do a CHECK here.
src/tests/slave_state_tests.cpp
<https://reviews.apache.org/r/6842/#comment24606>
I smell 'Option<string> rootDir;' instead of checking for empty string (more explicit). ;)
third_party/libprocess/include/stout/format.hpp
<https://reviews.apache.org/r/6842/#comment24601>
Thank you!
third_party/libprocess/include/stout/os.hpp
<https://reviews.apache.org/r/6842/#comment24602>
s/file/file: /
third_party/libprocess/include/stout/os.hpp
<https://reviews.apache.org/r/6842/#comment24603>
s/directory/directory:/
- Benjamin Hindman
On Sept. 12, 2012, 10:23 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6842/
> -----------------------------------------------------------
>
> (Updated Sept. 12, 2012, 10:23 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> And so it begins...
>
> I am discarding the old review associated with this (previously called path: https://reviews.apache.org/r/4944/), in favor of a fresh start.
>
> slave/state.{hpp,cpp} contains all the functions that deal with persistent state of the slave.
>
>
> This addresses bug MESOS-110.
> https://issues.apache.org/jira/browse/MESOS-110
>
>
> Diffs
> -----
>
> src/Makefile.am b73a024
> src/common/protobuf_utils.hpp PRE-CREATION
> src/slave/slave.hpp 7fbae8a
> src/slave/slave.cpp 4ea1db1
> src/slave/state.hpp PRE-CREATION
> src/slave/state.cpp PRE-CREATION
> src/tests/slave_state_tests.cpp PRE-CREATION
> third_party/libprocess/include/stout/format.hpp c746ad3
> third_party/libprocess/include/stout/os.hpp d08b0cb
>
> Diff: https://reviews.apache.org/r/6842/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Uno): Factoring out Slave State
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6842/#review11714
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Hindman
On Sept. 18, 2012, 7:21 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6842/
> -----------------------------------------------------------
>
> (Updated Sept. 18, 2012, 7:21 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> And so it begins...
>
> I am discarding the old review associated with this (previously called path: https://reviews.apache.org/r/4944/), in favor of a fresh start.
>
> slave/state.{hpp,cpp} contains all the functions that deal with persistent state of the slave.
>
>
> This addresses bug MESOS-110.
> https://issues.apache.org/jira/browse/MESOS-110
>
>
> Diffs
> -----
>
> src/Makefile.am 88cdd7c
> src/common/protobuf_utils.hpp PRE-CREATION
> src/slave/path.hpp PRE-CREATION
> src/slave/slave.hpp e5a0aea
> src/slave/slave.cpp b85f954
> src/slave/state.hpp PRE-CREATION
> src/slave/state.cpp PRE-CREATION
> src/tests/slave_state_tests.cpp PRE-CREATION
> third_party/libprocess/include/stout/format.hpp c746ad3
> third_party/libprocess/include/stout/os.hpp 5b909b9
>
> Diff: https://reviews.apache.org/r/6842/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Uno): Factoring out Slave State
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6842/
-----------------------------------------------------------
(Updated Sept. 18, 2012, 7:21 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
Refactored slave path utils into a separate file.
Rebased off apache/trunk
Description
-------
And so it begins...
I am discarding the old review associated with this (previously called path: https://reviews.apache.org/r/4944/), in favor of a fresh start.
slave/state.{hpp,cpp} contains all the functions that deal with persistent state of the slave.
This addresses bug MESOS-110.
https://issues.apache.org/jira/browse/MESOS-110
Diffs (updated)
-----
src/Makefile.am 88cdd7c
src/common/protobuf_utils.hpp PRE-CREATION
src/slave/path.hpp PRE-CREATION
src/slave/slave.hpp e5a0aea
src/slave/slave.cpp b85f954
src/slave/state.hpp PRE-CREATION
src/slave/state.cpp PRE-CREATION
src/tests/slave_state_tests.cpp PRE-CREATION
third_party/libprocess/include/stout/format.hpp c746ad3
third_party/libprocess/include/stout/os.hpp 5b909b9
Diff: https://reviews.apache.org/r/6842/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Slave Restart (Part Uno): Factoring out Slave State
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6842/
-----------------------------------------------------------
(Updated Sept. 13, 2012, 10:46 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
Bens' comments
Description
-------
And so it begins...
I am discarding the old review associated with this (previously called path: https://reviews.apache.org/r/4944/), in favor of a fresh start.
slave/state.{hpp,cpp} contains all the functions that deal with persistent state of the slave.
This addresses bug MESOS-110.
https://issues.apache.org/jira/browse/MESOS-110
Diffs (updated)
-----
src/Makefile.am b73a024
src/common/protobuf_utils.hpp PRE-CREATION
src/slave/slave.hpp 7fbae8a
src/slave/slave.cpp 4ea1db1
src/slave/state.hpp PRE-CREATION
src/slave/state.cpp PRE-CREATION
src/tests/slave_state_tests.cpp PRE-CREATION
third_party/libprocess/include/stout/format.hpp c746ad3
third_party/libprocess/include/stout/os.hpp d08b0cb
Diff: https://reviews.apache.org/r/6842/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Slave Restart (Part Uno): Factoring out Slave State
Posted by Vinod Kone <vi...@gmail.com>.
> On Sept. 13, 2012, 4:02 a.m., Ben Mahler wrote:
> > src/slave/slave.cpp, line 86
> > <https://reviews.apache.org/r/6842/diff/2/?file=153051#file153051line86>
> >
> > you moved this into protobuf utils, so kill this version
good catch. killed
> On Sept. 13, 2012, 4:02 a.m., Ben Mahler wrote:
> > src/slave/state.hpp, line 39
> > <https://reviews.apache.org/r/6842/diff/2/?file=153052#file153052line39>
> >
> > can you kill these from the header? any reason for exposing them, other than testing?
> >
> > I thought I would need some of these for file access, but I only would need the executor work directory from createUniqueExecutorWorkDirectory
These would be useful outside testing too. For eg: in the forthcoming Status Update Manager code.
> On Sept. 13, 2012, 4:02 a.m., Ben Mahler wrote:
> > src/slave/state.hpp, line 140
> > <https://reviews.apache.org/r/6842/diff/2/?file=153052#file153052line140>
> >
> > comment all these guys
Done. Though, I'm now having doubts about the usefulness of the helper functions to read/write stuff. Added a TODO to revisit them after I integrate with StatusUpdateManager.
> On Sept. 13, 2012, 4:02 a.m., Ben Mahler wrote:
> > src/slave/state.cpp, line 26
> > <https://reviews.apache.org/r/6842/diff/2/?file=153053#file153053line26>
> >
> > 80 line length limit on these
actually, now that these were moved to the cpp file and we have helpers, decided to revert back to my old stye of composing paths. in my opinion, the intention is clearer with composition.
> On Sept. 13, 2012, 4:02 a.m., Ben Mahler wrote:
> > src/slave/state.cpp, line 156
> > <https://reviews.apache.org/r/6842/diff/2/?file=153053#file153053line156>
> >
> > use the helper?
oops, done
> On Sept. 13, 2012, 4:02 a.m., Ben Mahler wrote:
> > src/slave/state.cpp, line 175
> > <https://reviews.apache.org/r/6842/diff/2/?file=153053#file153053line175>
> >
> > any ideas for reusing the helpers on these * globs?
this will end up being a bunch templatized functions akin to format. i will punt on this till we move to c++11.
> On Sept. 13, 2012, 4:02 a.m., Ben Mahler wrote:
> > src/slave/state.cpp, line 189
> > <https://reviews.apache.org/r/6842/diff/2/?file=153053#file153053line189>
> >
> > ditto
ditto
> On Sept. 13, 2012, 4:02 a.m., Ben Mahler wrote:
> > src/slave/state.cpp, line 201
> > <https://reviews.apache.org/r/6842/diff/2/?file=153053#file153053line201>
> >
> > s/result/run
'run' is used below.
> On Sept. 13, 2012, 4:02 a.m., Ben Mahler wrote:
> > src/slave/state.cpp, line 207
> > <https://reviews.apache.org/r/6842/diff/2/?file=153053#file153053line207>
> >
> > can kill this var, and just use run.get()
there are too many uses of run.get(), that made me want to extract it out, to make it cleaner
> On Sept. 13, 2012, 4:02 a.m., Ben Mahler wrote:
> > src/slave/state.cpp, line 277
> > <https://reviews.apache.org/r/6842/diff/2/?file=153053#file153053line277>
> >
> > inline os::mkdir in the CHECK?
see Ben's comment on my previous review (slave gc) regarding this. basically, since CHECK looks/acts like ASSERT, Ben wants to avoid having statements as argument.
> On Sept. 13, 2012, 4:02 a.m., Ben Mahler wrote:
> > src/slave/state.cpp, line 332
> > <https://reviews.apache.org/r/6842/diff/2/?file=153053#file153053line332>
> >
> > maybe strings::trim(result.get())?
we control the writes, so not needed?
> On Sept. 13, 2012, 4:02 a.m., Ben Mahler wrote:
> > src/slave/state.cpp, line 371
> > <https://reviews.apache.org/r/6842/diff/2/?file=153053#file153053line371>
> >
> > maybe strings::trim(result.get())
ditto
> On Sept. 13, 2012, 4:02 a.m., Ben Mahler wrote:
> > third_party/libprocess/include/stout/os.hpp, line 191
> > <https://reviews.apache.org/r/6842/diff/2/?file=153056#file153056line191>
> >
> > can you move these new functions into the newly created stout/fs.hpp?
i will do it once you commit it :)
> On Sept. 13, 2012, 4:02 a.m., Ben Mahler wrote:
> > third_party/libprocess/include/stout/os.hpp, line 195
> > <https://reviews.apache.org/r/6842/diff/2/?file=153056#file153056line195>
> >
> > let's stay away from modifying the underlying char array of a string: http://stackoverflow.com/questions/5729203/modifying-underlying-char-array-of-a-c-string-object
> >
> > create a char* copy instead?
it looks ugly after the fix, but hopefully correctness trump ugliness :)
> On Sept. 13, 2012, 4:02 a.m., Ben Mahler wrote:
> > third_party/libprocess/include/stout/os.hpp, line 416
> > <https://reviews.apache.org/r/6842/diff/2/?file=153056#file153056line416>
> >
> > ditto
ditto
> On Sept. 13, 2012, 4:02 a.m., Ben Mahler wrote:
> > third_party/libprocess/include/stout/os.hpp, line 739
> > <https://reviews.apache.org/r/6842/diff/2/?file=153056#file153056line739>
> >
> > seems cleaner if we change this to a Try<list<string>> and have empty list indicate no matches
agreed, done
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6842/#review11450
-----------------------------------------------------------
On Sept. 12, 2012, 10:23 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6842/
> -----------------------------------------------------------
>
> (Updated Sept. 12, 2012, 10:23 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> And so it begins...
>
> I am discarding the old review associated with this (previously called path: https://reviews.apache.org/r/4944/), in favor of a fresh start.
>
> slave/state.{hpp,cpp} contains all the functions that deal with persistent state of the slave.
>
>
> This addresses bug MESOS-110.
> https://issues.apache.org/jira/browse/MESOS-110
>
>
> Diffs
> -----
>
> src/Makefile.am b73a024
> src/common/protobuf_utils.hpp PRE-CREATION
> src/slave/slave.hpp 7fbae8a
> src/slave/slave.cpp 4ea1db1
> src/slave/state.hpp PRE-CREATION
> src/slave/state.cpp PRE-CREATION
> src/tests/slave_state_tests.cpp PRE-CREATION
> third_party/libprocess/include/stout/format.hpp c746ad3
> third_party/libprocess/include/stout/os.hpp d08b0cb
>
> Diff: https://reviews.apache.org/r/6842/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Uno): Factoring out Slave State
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6842/#review11450
-----------------------------------------------------------
rebase looks good!
a more thorough review, have yet to look at the tests
src/slave/slave.cpp
<https://reviews.apache.org/r/6842/#comment24495>
you moved this into protobuf utils, so kill this version
src/slave/state.hpp
<https://reviews.apache.org/r/6842/#comment24500>
can you kill these from the header? any reason for exposing them, other than testing?
I thought I would need some of these for file access, but I only would need the executor work directory from createUniqueExecutorWorkDirectory
src/slave/state.hpp
<https://reviews.apache.org/r/6842/#comment24501>
two space separation on these guys, no?
src/slave/state.hpp
<https://reviews.apache.org/r/6842/#comment24515>
comment all these guys
src/slave/state.cpp
<https://reviews.apache.org/r/6842/#comment24502>
80 line length limit on these
src/slave/state.cpp
<https://reviews.apache.org/r/6842/#comment24503>
use the helper?
src/slave/state.cpp
<https://reviews.apache.org/r/6842/#comment24508>
any ideas for reusing the helpers on these * globs?
src/slave/state.cpp
<https://reviews.apache.org/r/6842/#comment24506>
ditto
src/slave/state.cpp
<https://reviews.apache.org/r/6842/#comment24511>
s/rPath/runPath
src/slave/state.cpp
<https://reviews.apache.org/r/6842/#comment24509>
s/result/run
src/slave/state.cpp
<https://reviews.apache.org/r/6842/#comment24510>
can kill this var, and just use run.get()
src/slave/state.cpp
<https://reviews.apache.org/r/6842/#comment24512>
s/tPath/taskPath
src/slave/state.cpp
<https://reviews.apache.org/r/6842/#comment24513>
inline the basename here, instead of storing it in a variable?
src/slave/state.cpp
<https://reviews.apache.org/r/6842/#comment24516>
move this comment to the header
src/slave/state.cpp
<https://reviews.apache.org/r/6842/#comment24514>
inline os::mkdir in the CHECK?
src/slave/state.cpp
<https://reviews.apache.org/r/6842/#comment24517>
move this down to where you use it
src/slave/state.cpp
<https://reviews.apache.org/r/6842/#comment24518>
maybe strings::trim(result.get())?
src/slave/state.cpp
<https://reviews.apache.org/r/6842/#comment24519>
maybe strings::trim(result.get())
third_party/libprocess/include/stout/os.hpp
<https://reviews.apache.org/r/6842/#comment24496>
can you move these new functions into the newly created stout/fs.hpp?
third_party/libprocess/include/stout/os.hpp
<https://reviews.apache.org/r/6842/#comment24497>
let's stay away from modifying the underlying char array of a string: http://stackoverflow.com/questions/5729203/modifying-underlying-char-array-of-a-c-string-object
create a char* copy instead?
third_party/libprocess/include/stout/os.hpp
<https://reviews.apache.org/r/6842/#comment24498>
ditto
third_party/libprocess/include/stout/os.hpp
<https://reviews.apache.org/r/6842/#comment24499>
seems cleaner if we change this to a Try<list<string>> and have empty list indicate no matches
- Ben Mahler
On Sept. 12, 2012, 10:23 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6842/
> -----------------------------------------------------------
>
> (Updated Sept. 12, 2012, 10:23 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Description
> -------
>
> And so it begins...
>
> I am discarding the old review associated with this (previously called path: https://reviews.apache.org/r/4944/), in favor of a fresh start.
>
> slave/state.{hpp,cpp} contains all the functions that deal with persistent state of the slave.
>
>
> This addresses bug MESOS-110.
> https://issues.apache.org/jira/browse/MESOS-110
>
>
> Diffs
> -----
>
> src/Makefile.am b73a024
> src/common/protobuf_utils.hpp PRE-CREATION
> src/slave/slave.hpp 7fbae8a
> src/slave/slave.cpp 4ea1db1
> src/slave/state.hpp PRE-CREATION
> src/slave/state.cpp PRE-CREATION
> src/tests/slave_state_tests.cpp PRE-CREATION
> third_party/libprocess/include/stout/format.hpp c746ad3
> third_party/libprocess/include/stout/os.hpp d08b0cb
>
> Diff: https://reviews.apache.org/r/6842/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Slave Restart (Part Uno): Factoring out Slave State
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6842/
-----------------------------------------------------------
(Updated Sept. 12, 2012, 10:23 p.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
benm's comments.
also, diff'ed against apache/trunk, after a huuuuuge rebase. please check carefully :)
Description
-------
And so it begins...
I am discarding the old review associated with this (previously called path: https://reviews.apache.org/r/4944/), in favor of a fresh start.
slave/state.{hpp,cpp} contains all the functions that deal with persistent state of the slave.
This addresses bug MESOS-110.
https://issues.apache.org/jira/browse/MESOS-110
Diffs (updated)
-----
src/Makefile.am b73a024
src/common/protobuf_utils.hpp PRE-CREATION
src/slave/slave.hpp 7fbae8a
src/slave/slave.cpp 4ea1db1
src/slave/state.hpp PRE-CREATION
src/slave/state.cpp PRE-CREATION
src/tests/slave_state_tests.cpp PRE-CREATION
third_party/libprocess/include/stout/format.hpp c746ad3
third_party/libprocess/include/stout/os.hpp d08b0cb
Diff: https://reviews.apache.org/r/6842/diff/
Testing
-------
make check
Thanks,
Vinod Kone