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 2014/03/12 01:45:33 UTC
Review Request 19085: Implemented optional strictness in the Registrar.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19085/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Vinod Kone.
Bugs: MESOS-764
https://issues.apache.org/jira/browse/MESOS-764
Repository: mesos-git
Description
-------
This adds the notion of "non-strict" operations in the Registrar. Operations that are not "strict" are always permitted.
This allows the state to be bootstrapped from the current cluster state and is essential for those upgrading a running cluster from a pre-Registrar version of Mesos.
I have included a TODO that discusses a simplified design of the Registrar per a chat with benh. I'd like to implement this simplified design in a subsequent change so I'd like to hear if there are any potential issues. The new design would mean pushing Operations out to a header and allowing callers to use Operations directly.
Diffs
-----
src/master/registrar.hpp 987a63b33bd26add3253f25f1471dd4a73471830
src/master/registrar.cpp d30172cee8525a80661135e6554bc0d78c7e314d
src/tests/registrar_tests.cpp 95a6b53e479ea9131aca9b39f096e6dc9e6848f8
Diff: https://reviews.apache.org/r/19085/diff/
Testing
-------
make check
I've parameterized the existing tests for strict and non-strict operation.
I've also added a test to ensure that the state can be bootstrapped.
Thanks,
Ben Mahler
Re: Review Request 19085: Implemented optional strictness in the Registrar.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19085/#review36989
-----------------------------------------------------------
src/master/registrar.hpp
<https://reviews.apache.org/r/19085/#comment68218>
I think we should be passing in flags via the constructor and then having the registrar call the Operations::operator () with a 'bool strict' parameter. This should reduce the total code changes and will let us easily make the "operations" be lambdas in the future too.
- Benjamin Hindman
On March 12, 2014, 12:45 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19085/
> -----------------------------------------------------------
>
> (Updated March 12, 2014, 12:45 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Bugs: MESOS-764
> https://issues.apache.org/jira/browse/MESOS-764
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This adds the notion of "non-strict" operations in the Registrar. Operations that are not "strict" are always permitted.
>
> This allows the state to be bootstrapped from the current cluster state and is essential for those upgrading a running cluster from a pre-Registrar version of Mesos.
>
> I have included a TODO that discusses a simplified design of the Registrar per a chat with benh. I'd like to implement this simplified design in a subsequent change so I'd like to hear if there are any potential issues. The new design would mean pushing Operations out to a header and allowing callers to use Operations directly.
>
>
> Diffs
> -----
>
> src/master/registrar.hpp 987a63b33bd26add3253f25f1471dd4a73471830
> src/master/registrar.cpp d30172cee8525a80661135e6554bc0d78c7e314d
> src/tests/registrar_tests.cpp 95a6b53e479ea9131aca9b39f096e6dc9e6848f8
>
> Diff: https://reviews.apache.org/r/19085/diff/
>
>
> Testing
> -------
>
> make check
>
> I've parameterized the existing tests for strict and non-strict operation.
> I've also added a test to ensure that the state can be bootstrapped.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 19085: Implemented optional strictness in the Registrar.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19085/#review37277
-----------------------------------------------------------
Ship it!
src/master/registrar.cpp
<https://reviews.apache.org/r/19085/#comment68695>
Can we update the comment?
- Benjamin Hindman
On March 14, 2014, 6:01 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19085/
> -----------------------------------------------------------
>
> (Updated March 14, 2014, 6:01 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Bugs: MESOS-764
> https://issues.apache.org/jira/browse/MESOS-764
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This adds the notion of "non-strict" operations in the Registrar. Operations that are not "strict" are always permitted.
>
> This allows the state to be bootstrapped from the current cluster state and is essential for those upgrading a running cluster from a pre-Registrar version of Mesos.
>
> I have included a TODO that discusses a simplified design of the Registrar per a chat with benh. I'd like to implement this simplified design in a subsequent change so I'd like to hear if there are any potential issues. The new design would mean pushing Operations out to a header and allowing callers to use Operations directly.
>
>
> Diffs
> -----
>
> src/local/local.cpp e31c19c1bd0bc527275b818a8ce2bc1d54d8cb89
> src/master/main.cpp a99fb6494f1dbbfce91698d5aaded2b982bbaab5
> src/master/registrar.hpp 987a63b33bd26add3253f25f1471dd4a73471830
> src/master/registrar.cpp d30172cee8525a80661135e6554bc0d78c7e314d
> src/tests/cluster.hpp 24bb7508955599731f4a06eeffc15441afbe1721
> src/tests/registrar_tests.cpp 95a6b53e479ea9131aca9b39f096e6dc9e6848f8
>
> Diff: https://reviews.apache.org/r/19085/diff/
>
>
> Testing
> -------
>
> make check
>
> I've parameterized the existing tests for strict and non-strict operation.
> I've also added a test to ensure that the state can be bootstrapped.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 19085: Implemented optional strictness in the Registrar.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19085/
-----------------------------------------------------------
(Updated March 14, 2014, 6:01 p.m.)
Review request for mesos, Benjamin Hindman and Vinod Kone.
Changes
-------
Moved strict to be passed via Flags.
Depends on r/19135.
Bugs: MESOS-764
https://issues.apache.org/jira/browse/MESOS-764
Repository: mesos-git
Description
-------
This adds the notion of "non-strict" operations in the Registrar. Operations that are not "strict" are always permitted.
This allows the state to be bootstrapped from the current cluster state and is essential for those upgrading a running cluster from a pre-Registrar version of Mesos.
I have included a TODO that discusses a simplified design of the Registrar per a chat with benh. I'd like to implement this simplified design in a subsequent change so I'd like to hear if there are any potential issues. The new design would mean pushing Operations out to a header and allowing callers to use Operations directly.
Diffs (updated)
-----
src/local/local.cpp e31c19c1bd0bc527275b818a8ce2bc1d54d8cb89
src/master/main.cpp a99fb6494f1dbbfce91698d5aaded2b982bbaab5
src/master/registrar.hpp 987a63b33bd26add3253f25f1471dd4a73471830
src/master/registrar.cpp d30172cee8525a80661135e6554bc0d78c7e314d
src/tests/cluster.hpp 24bb7508955599731f4a06eeffc15441afbe1721
src/tests/registrar_tests.cpp 95a6b53e479ea9131aca9b39f096e6dc9e6848f8
Diff: https://reviews.apache.org/r/19085/diff/
Testing
-------
make check
I've parameterized the existing tests for strict and non-strict operation.
I've also added a test to ensure that the state can be bootstrapped.
Thanks,
Ben Mahler
Re: Review Request 19085: Implemented optional strictness in the Registrar.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19085/#review36884
-----------------------------------------------------------
Patch looks great!
Reviews applied: [19085]
All tests passed.
- Mesos ReviewBot
On March 12, 2014, 12:45 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19085/
> -----------------------------------------------------------
>
> (Updated March 12, 2014, 12:45 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Bugs: MESOS-764
> https://issues.apache.org/jira/browse/MESOS-764
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This adds the notion of "non-strict" operations in the Registrar. Operations that are not "strict" are always permitted.
>
> This allows the state to be bootstrapped from the current cluster state and is essential for those upgrading a running cluster from a pre-Registrar version of Mesos.
>
> I have included a TODO that discusses a simplified design of the Registrar per a chat with benh. I'd like to implement this simplified design in a subsequent change so I'd like to hear if there are any potential issues. The new design would mean pushing Operations out to a header and allowing callers to use Operations directly.
>
>
> Diffs
> -----
>
> src/master/registrar.hpp 987a63b33bd26add3253f25f1471dd4a73471830
> src/master/registrar.cpp d30172cee8525a80661135e6554bc0d78c7e314d
> src/tests/registrar_tests.cpp 95a6b53e479ea9131aca9b39f096e6dc9e6848f8
>
> Diff: https://reviews.apache.org/r/19085/diff/
>
>
> Testing
> -------
>
> make check
>
> I've parameterized the existing tests for strict and non-strict operation.
> I've also added a test to ensure that the state can be bootstrapped.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 19085: Implemented optional strictness in the Registrar.
Posted by Ben Mahler <be...@gmail.com>.
> On March 12, 2014, 4:17 p.m., Dominic Hamon wrote:
> > src/tests/registrar_tests.cpp, line 140
> > <https://reviews.apache.org/r/19085/diff/1/?file=516862#file516862line140>
> >
> > if we had AWAIT_TRUE and AWAIT_FALSE this would be cleaner.
Agreed! Will punt on adding this.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19085/#review36925
-----------------------------------------------------------
On March 12, 2014, 12:45 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19085/
> -----------------------------------------------------------
>
> (Updated March 12, 2014, 12:45 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Bugs: MESOS-764
> https://issues.apache.org/jira/browse/MESOS-764
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This adds the notion of "non-strict" operations in the Registrar. Operations that are not "strict" are always permitted.
>
> This allows the state to be bootstrapped from the current cluster state and is essential for those upgrading a running cluster from a pre-Registrar version of Mesos.
>
> I have included a TODO that discusses a simplified design of the Registrar per a chat with benh. I'd like to implement this simplified design in a subsequent change so I'd like to hear if there are any potential issues. The new design would mean pushing Operations out to a header and allowing callers to use Operations directly.
>
>
> Diffs
> -----
>
> src/master/registrar.hpp 987a63b33bd26add3253f25f1471dd4a73471830
> src/master/registrar.cpp d30172cee8525a80661135e6554bc0d78c7e314d
> src/tests/registrar_tests.cpp 95a6b53e479ea9131aca9b39f096e6dc9e6848f8
>
> Diff: https://reviews.apache.org/r/19085/diff/
>
>
> Testing
> -------
>
> make check
>
> I've parameterized the existing tests for strict and non-strict operation.
> I've also added a test to ensure that the state can be bootstrapped.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 19085: Implemented optional strictness in the Registrar.
Posted by Ben Mahler <be...@gmail.com>.
> On March 12, 2014, 4:17 p.m., Dominic Hamon wrote:
> > src/master/registrar.hpp, line 59
> > <https://reviews.apache.org/r/19085/diff/1/?file=516860#file516860line59>
> >
> > an enum with STRICT/NONSTRICT would be clearer from an API point of view.
The callers will be directly using a flag:
registrar.admit(slave, flags.registry_strict_operations)
An enum would be more clumsy in the caller.
> On March 12, 2014, 4:17 p.m., Dominic Hamon wrote:
> > src/master/registrar.cpp, line 158
> > <https://reviews.apache.org/r/19085/diff/1/?file=516861#file516861line158>
> >
> > no need for else as you return in the if.
I want to make the difference between strict/non-strict operations very clear for those reading this code, so I would prefer an if / else to make the two cases clear.
> On March 12, 2014, 4:17 p.m., Dominic Hamon wrote:
> > src/tests/registrar_tests.cpp, line 176
> > <https://reviews.apache.org/r/19085/diff/1/?file=516862#file516862line176>
> >
> > AWAIT_EQ(!strict, registrar.readmit(info2, strict));
I originally had this. While it is technically correct, it requires some thought to understand. Again in this case I want to make strict / non-strict cases very explicit.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19085/#review36925
-----------------------------------------------------------
On March 12, 2014, 12:45 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19085/
> -----------------------------------------------------------
>
> (Updated March 12, 2014, 12:45 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Bugs: MESOS-764
> https://issues.apache.org/jira/browse/MESOS-764
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This adds the notion of "non-strict" operations in the Registrar. Operations that are not "strict" are always permitted.
>
> This allows the state to be bootstrapped from the current cluster state and is essential for those upgrading a running cluster from a pre-Registrar version of Mesos.
>
> I have included a TODO that discusses a simplified design of the Registrar per a chat with benh. I'd like to implement this simplified design in a subsequent change so I'd like to hear if there are any potential issues. The new design would mean pushing Operations out to a header and allowing callers to use Operations directly.
>
>
> Diffs
> -----
>
> src/master/registrar.hpp 987a63b33bd26add3253f25f1471dd4a73471830
> src/master/registrar.cpp d30172cee8525a80661135e6554bc0d78c7e314d
> src/tests/registrar_tests.cpp 95a6b53e479ea9131aca9b39f096e6dc9e6848f8
>
> Diff: https://reviews.apache.org/r/19085/diff/
>
>
> Testing
> -------
>
> make check
>
> I've parameterized the existing tests for strict and non-strict operation.
> I've also added a test to ensure that the state can be bootstrapped.
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 19085: Implemented optional strictness in the Registrar.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19085/#review36925
-----------------------------------------------------------
src/master/registrar.hpp
<https://reviews.apache.org/r/19085/#comment68112>
an enum with STRICT/NONSTRICT would be clearer from an API point of view.
src/master/registrar.cpp
<https://reviews.apache.org/r/19085/#comment68114>
this would also make it easier to add unit tests for each operation.
src/master/registrar.cpp
<https://reviews.apache.org/r/19085/#comment68113>
no need for else as you return in the if.
src/master/registrar.cpp
<https://reviews.apache.org/r/19085/#comment68115>
no need for else as you return in the if.
src/master/registrar.cpp
<https://reviews.apache.org/r/19085/#comment68116>
no need for else as you return in the if.
src/tests/registrar_tests.cpp
<https://reviews.apache.org/r/19085/#comment68117>
if we had AWAIT_TRUE and AWAIT_FALSE this would be cleaner.
src/tests/registrar_tests.cpp
<https://reviews.apache.org/r/19085/#comment68118>
AWAIT_EQ(!strict, registrar.readmit(info2, strict));
- Dominic Hamon
On March 11, 2014, 5:45 p.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19085/
> -----------------------------------------------------------
>
> (Updated March 11, 2014, 5:45 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Vinod Kone.
>
>
> Bugs: MESOS-764
> https://issues.apache.org/jira/browse/MESOS-764
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> This adds the notion of "non-strict" operations in the Registrar. Operations that are not "strict" are always permitted.
>
> This allows the state to be bootstrapped from the current cluster state and is essential for those upgrading a running cluster from a pre-Registrar version of Mesos.
>
> I have included a TODO that discusses a simplified design of the Registrar per a chat with benh. I'd like to implement this simplified design in a subsequent change so I'd like to hear if there are any potential issues. The new design would mean pushing Operations out to a header and allowing callers to use Operations directly.
>
>
> Diffs
> -----
>
> src/master/registrar.hpp 987a63b33bd26add3253f25f1471dd4a73471830
> src/master/registrar.cpp d30172cee8525a80661135e6554bc0d78c7e314d
> src/tests/registrar_tests.cpp 95a6b53e479ea9131aca9b39f096e6dc9e6848f8
>
> Diff: https://reviews.apache.org/r/19085/diff/
>
>
> Testing
> -------
>
> make check
>
> I've parameterized the existing tests for strict and non-strict operation.
> I've also added a test to ensure that the state can be bootstrapped.
>
>
> Thanks,
>
> Ben Mahler
>
>