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 2013/12/02 02:28:05 UTC
Re: Review Request 14902: Catch-up Replicated Log 2: libprocessify the
coordinator.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14902/#review29573
-----------------------------------------------------------
Very nice, I will let benh give a ship it for this, looks good to me!
src/log/coordinator.cpp
<https://reviews.apache.org/r/14902/#comment56943>
s/committed/committed (learned)/ might be helpful to those familiar with paxos but unfamiliar with our terminology, ditto elsewhere
src/log/coordinator.cpp
<https://reviews.apache.org/r/14902/#comment56948>
Perhaps a note here (and/or in the header) that one can only demote() once the co-ordinator has been fully elected?
That is:
elect();
demote(); // Possibly fails.
elect().get();
demote(); // Expected usage.
Ditto for adding a note with respect to not being able to demote while a write is "in progress".
src/log/coordinator.cpp
<https://reviews.apache.org/r/14902/#comment56940>
Can you place a CHECK_EQ(state, INITIAL) before setting the state to ELECTING?
src/log/coordinator.cpp
<https://reviews.apache.org/r/14902/#comment56945>
Can you move demote() below all of the election related functions?
src/log/coordinator.cpp
<https://reviews.apache.org/r/14902/#comment56941>
CHECK_EQ(state, ELECTED) before this?
src/log/coordinator.cpp
<https://reviews.apache.org/r/14902/#comment56946>
CHECK_LE?
src/log/coordinator.cpp
<https://reviews.apache.org/r/14902/#comment56947>
CHECK_EQ?
src/log/coordinator.cpp
<https://reviews.apache.org/r/14902/#comment56950>
CHECK_EQ?
src/log/coordinator.cpp
<https://reviews.apache.org/r/14902/#comment56952>
s/Aborted/Discarded/ on these continuations?
src/log/coordinator.cpp
<https://reviews.apache.org/r/14902/#comment56951>
CHECK_EQ?
src/log/coordinator.cpp
<https://reviews.apache.org/r/14902/#comment56959>
Why not use these same blocks to delineate the election and demotion parts of the implementation as well?
src/log/coordinator.cpp
<https://reviews.apache.org/r/14902/#comment56954>
CHECK_EQ?
src/log/coordinator.cpp
<https://reviews.apache.org/r/14902/#comment56955>
CHECK_LE?
src/log/coordinator.cpp
<https://reviews.apache.org/r/14902/#comment56956>
CHECK_EQ on these?
- Ben Mahler
On Nov. 25, 2013, 5:53 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14902/
> -----------------------------------------------------------
>
> (Updated Nov. 25, 2013, 5:53 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-736
> https://issues.apache.org/jira/browse/MESOS-736
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Libprocessify the coordinator.
>
>
> Diffs
> -----
>
> src/log/coordinator.hpp 3f6fb7c
> src/log/coordinator.cpp 6e6466f
>
> Diff: https://reviews.apache.org/r/14902/diff/
>
>
> Testing
> -------
>
> bin/mesos-tests.sh --gtest_filter=*CoordinatorTest*:*LogTest*:*ReplicaTest* --gtest_repeat=100
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 14902: Catch-up Replicated Log 2: libprocessify the
coordinator.
Posted by Jie Yu <yu...@gmail.com>.
> On Dec. 2, 2013, 1:28 a.m., Ben Mahler wrote:
> > src/log/coordinator.cpp, line 160
> > <https://reviews.apache.org/r/14902/diff/9/?file=390614#file390614line160>
> >
> > Can you place a CHECK_EQ(state, INITIAL) before setting the state to ELECTING?
Done.
> On Dec. 2, 2013, 1:28 a.m., Ben Mahler wrote:
> > src/log/coordinator.cpp, lines 178-190
> > <https://reviews.apache.org/r/14902/diff/9/?file=390614#file390614line178>
> >
> > Can you move demote() below all of the election related functions?
Done.
> On Dec. 2, 2013, 1:28 a.m., Ben Mahler wrote:
> > src/log/coordinator.cpp, line 62
> > <https://reviews.apache.org/r/14902/diff/9/?file=390614#file390614line62>
> >
> > s/committed/committed (learned)/ might be helpful to those familiar with paxos but unfamiliar with our terminology, ditto elsewhere
Since I will move these comments else where in the next patch, I will do that in the next patch.
> On Dec. 2, 2013, 1:28 a.m., Ben Mahler wrote:
> > src/log/coordinator.cpp, lines 67-69
> > <https://reviews.apache.org/r/14902/diff/9/?file=390614#file390614line67>
> >
> > Perhaps a note here (and/or in the header) that one can only demote() once the co-ordinator has been fully elected?
> >
> > That is:
> >
> > elect();
> > demote(); // Possibly fails.
> >
> > elect().get();
> > demote(); // Expected usage.
> >
> > Ditto for adding a note with respect to not being able to demote while a write is "in progress".
ditto
> On Dec. 2, 2013, 1:28 a.m., Ben Mahler wrote:
> > src/log/coordinator.cpp, line 188
> > <https://reviews.apache.org/r/14902/diff/9/?file=390614#file390614line188>
> >
> > CHECK_EQ(state, ELECTED) before this?
Done.
> On Dec. 2, 2013, 1:28 a.m., Ben Mahler wrote:
> > src/log/coordinator.cpp, line 225
> > <https://reviews.apache.org/r/14902/diff/9/?file=390614#file390614line225>
> >
> > CHECK_LE?
Done.
> On Dec. 2, 2013, 1:28 a.m., Ben Mahler wrote:
> > src/log/coordinator.cpp, line 281
> > <https://reviews.apache.org/r/14902/diff/9/?file=390614#file390614line281>
> >
> > CHECK_EQ?
Done.
> On Dec. 2, 2013, 1:28 a.m., Ben Mahler wrote:
> > src/log/coordinator.cpp, line 293
> > <https://reviews.apache.org/r/14902/diff/9/?file=390614#file390614line293>
> >
> > CHECK_EQ?
Done.
> On Dec. 2, 2013, 1:28 a.m., Ben Mahler wrote:
> > src/log/coordinator.cpp, line 298
> > <https://reviews.apache.org/r/14902/diff/9/?file=390614#file390614line298>
> >
> > s/Aborted/Discarded/ on these continuations?
I would prefer the old name because it's more clear that we're gonna cancel the election.
> On Dec. 2, 2013, 1:28 a.m., Ben Mahler wrote:
> > src/log/coordinator.cpp, line 302
> > <https://reviews.apache.org/r/14902/diff/9/?file=390614#file390614line302>
> >
> > CHECK_EQ?
Done.
> On Dec. 2, 2013, 1:28 a.m., Ben Mahler wrote:
> > src/log/coordinator.cpp, lines 307-309
> > <https://reviews.apache.org/r/14902/diff/9/?file=390614#file390614line307>
> >
> > Why not use these same blocks to delineate the election and demotion parts of the implementation as well?
Done
> On Dec. 2, 2013, 1:28 a.m., Ben Mahler wrote:
> > src/log/coordinator.cpp, line 381
> > <https://reviews.apache.org/r/14902/diff/9/?file=390614#file390614line381>
> >
> > CHECK_EQ?
Done.
> On Dec. 2, 2013, 1:28 a.m., Ben Mahler wrote:
> > src/log/coordinator.cpp, line 412
> > <https://reviews.apache.org/r/14902/diff/9/?file=390614#file390614line412>
> >
> > CHECK_LE?
Done.
> On Dec. 2, 2013, 1:28 a.m., Ben Mahler wrote:
> > src/log/coordinator.cpp, lines 453-471
> > <https://reviews.apache.org/r/14902/diff/9/?file=390614#file390614line453>
> >
> > CHECK_EQ on these?
Done.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14902/#review29573
-----------------------------------------------------------
On Nov. 25, 2013, 5:53 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14902/
> -----------------------------------------------------------
>
> (Updated Nov. 25, 2013, 5:53 a.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Bugs: MESOS-736
> https://issues.apache.org/jira/browse/MESOS-736
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Libprocessify the coordinator.
>
>
> Diffs
> -----
>
> src/log/coordinator.hpp 3f6fb7c
> src/log/coordinator.cpp 6e6466f
>
> Diff: https://reviews.apache.org/r/14902/diff/
>
>
> Testing
> -------
>
> bin/mesos-tests.sh --gtest_filter=*CoordinatorTest*:*LogTest*:*ReplicaTest* --gtest_repeat=100
>
>
> Thanks,
>
> Jie Yu
>
>