You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Ben Mahler <be...@gmail.com> on 2012/09/01 02:19:01 UTC
Re: 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/#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. 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
>
>