You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Jiang Yan Xu <ya...@jxu.me> on 2014/03/28 22:42:44 UTC
Re: Review Request 19762: Improved Registrar performance by using a hashset
on SlaveIDs.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19762/
-----------------------------------------------------------
(Updated March 28, 2014, 2:42 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
Minor renaming.
Summary (updated)
-----------------
Improved Registrar performance by using a hashset on SlaveIDs.
Bugs: MESOS-1155
https://issues.apache.org/jira/browse/MESOS-1155
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
src/master/master.hpp eb8cd73e9e8f26a7438070ca2bb078bd2d1ec480
src/master/registrar.hpp 0d659c57d55f7d054e106477c814d921479e3177
src/master/registrar.cpp 9c0537d86036dec3e6a0fac84e167d9131242e0c
Diff: https://reviews.apache.org/r/19762/diff/
Testing
-------
mesos-tests.sh –verbose –gtest_filter=“RegistrarTest*” –gtest_also_run_disabled_tests
On test machines this reduces the time for applying 20,000 (re)admit operations from 30secs to < 100ms
Thanks,
Jiang Yan Xu
Re: Review Request 19762: Improved Registrar performance by using a hashset
on SlaveIDs.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19762/#review38985
-----------------------------------------------------------
Patch looks great!
Reviews applied: [19762]
All tests passed.
- Mesos ReviewBot
On March 28, 2014, 9:42 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19762/
> -----------------------------------------------------------
>
> (Updated March 28, 2014, 9:42 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1155
> https://issues.apache.org/jira/browse/MESOS-1155
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/master.hpp eb8cd73e9e8f26a7438070ca2bb078bd2d1ec480
> src/master/registrar.hpp 0d659c57d55f7d054e106477c814d921479e3177
> src/master/registrar.cpp 9c0537d86036dec3e6a0fac84e167d9131242e0c
>
> Diff: https://reviews.apache.org/r/19762/diff/
>
>
> Testing
> -------
>
> mesos-tests.sh –verbose –gtest_filter=“RegistrarTest*” –gtest_also_run_disabled_tests
> On test machines this reduces the time for applying 20,000 (re)admit operations from 30secs to < 100ms
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 19762: Improved Registrar performance by using a hashset
on SlaveIDs.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19762/#review39205
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
On April 1, 2014, 1:13 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19762/
> -----------------------------------------------------------
>
> (Updated April 1, 2014, 1:13 a.m.)
>
>
> Review request for mesos, Ben Mahler and Vinod Kone.
>
>
> Bugs: MESOS-1155
> https://issues.apache.org/jira/browse/MESOS-1155
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/master.hpp eb8cd73e9e8f26a7438070ca2bb078bd2d1ec480
> src/master/registrar.hpp 0d659c57d55f7d054e106477c814d921479e3177
> src/master/registrar.cpp 9c0537d86036dec3e6a0fac84e167d9131242e0c
>
> Diff: https://reviews.apache.org/r/19762/diff/
>
>
> Testing
> -------
>
> mesos-tests.sh –verbose –gtest_filter=“RegistrarTest*” –gtest_also_run_disabled_tests
> On test machines this reduces the time for applying 20,000 (re)admit operations from 30secs to < 100ms
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 19762: Improved Registrar performance by using a hashset
on SlaveIDs.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19762/#review39574
-----------------------------------------------------------
Patch looks great!
Reviews applied: [19762]
All tests passed.
- Mesos ReviewBot
On April 3, 2014, 10:30 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19762/
> -----------------------------------------------------------
>
> (Updated April 3, 2014, 10:30 p.m.)
>
>
> Review request for mesos, Ben Mahler and Vinod Kone.
>
>
> Bugs: MESOS-1155
> https://issues.apache.org/jira/browse/MESOS-1155
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/master.hpp fef59c9971fcb3a5a33d7c94210b87edacb5719b
> src/master/registrar.hpp 0d659c57d55f7d054e106477c814d921479e3177
> src/master/registrar.cpp 9c0537d86036dec3e6a0fac84e167d9131242e0c
>
> Diff: https://reviews.apache.org/r/19762/diff/
>
>
> Testing
> -------
>
> mesos-tests.sh –verbose –gtest_filter=“RegistrarTest*” –gtest_also_run_disabled_tests
> On test machines this reduces the time for applying 20,000 (re)admit operations from 30secs to < 100ms
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 19762: Improved Registrar performance by using a hashset
on SlaveIDs.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19762/#review40338
-----------------------------------------------------------
Ship it!
Looks great, thanks Yan!
src/master/registrar.hpp
<https://reviews.apache.org/r/19762/#comment73342>
Probably we don't need this much detail here since we've already explained it below, you could just say:
"Make Operation generic so that we can apply them against a generic "batch operation applier" abstraction, see TODO below for more details."
src/master/registrar.hpp
<https://reviews.apache.org/r/19762/#comment73341>
Double quotes here but single quotes above?
src/master/registrar.hpp
<https://reviews.apache.org/r/19762/#comment73340>
s/to us/us to/
s/new features/new state variables/
And please end with period. :)
src/master/registrar.hpp
<https://reviews.apache.org/r/19762/#comment73343>
Rather than naming it, maybe just describe what it will do:
Add a generic abstraction for applying batches of operations against State Variables, the Registrar and other components could leverage this. This abstraction would be templatized to take the type, along with any accumulators: ...
- Ben Mahler
On April 14, 2014, 6:49 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19762/
> -----------------------------------------------------------
>
> (Updated April 14, 2014, 6:49 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-1155
> https://issues.apache.org/jira/browse/MESOS-1155
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/master.hpp fef59c9971fcb3a5a33d7c94210b87edacb5719b
> src/master/registrar.hpp 0d659c57d55f7d054e106477c814d921479e3177
> src/master/registrar.cpp 9c0537d86036dec3e6a0fac84e167d9131242e0c
>
> Diff: https://reviews.apache.org/r/19762/diff/
>
>
> Testing
> -------
>
> mesos-tests.sh –verbose –gtest_filter=“RegistrarTest*” –gtest_also_run_disabled_tests
> On test machines this reduces the time for applying 20,000 (re)admit operations from 30secs to < 100ms
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 19762: Improved Registrar performance by using a hashset
on SlaveIDs.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19762/#review40395
-----------------------------------------------------------
Patch looks great!
Reviews applied: [19762]
All tests passed.
- Mesos ReviewBot
On April 14, 2014, 6:49 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19762/
> -----------------------------------------------------------
>
> (Updated April 14, 2014, 6:49 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Ben Mahler.
>
>
> Bugs: MESOS-1155
> https://issues.apache.org/jira/browse/MESOS-1155
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/master.hpp fef59c9971fcb3a5a33d7c94210b87edacb5719b
> src/master/registrar.hpp 0d659c57d55f7d054e106477c814d921479e3177
> src/master/registrar.cpp 9c0537d86036dec3e6a0fac84e167d9131242e0c
>
> Diff: https://reviews.apache.org/r/19762/diff/
>
>
> Testing
> -------
>
> mesos-tests.sh –verbose –gtest_filter=“RegistrarTest*” –gtest_also_run_disabled_tests
> On test machines this reduces the time for applying 20,000 (re)admit operations from 30secs to < 100ms
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 19762: Improved Registrar performance by using a hashset
on SlaveIDs.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19762/
-----------------------------------------------------------
(Updated April 14, 2014, 11:49 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
Changes
-------
BenM's comments.
Bugs: MESOS-1155
https://issues.apache.org/jira/browse/MESOS-1155
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
src/master/master.hpp fef59c9971fcb3a5a33d7c94210b87edacb5719b
src/master/registrar.hpp 0d659c57d55f7d054e106477c814d921479e3177
src/master/registrar.cpp 9c0537d86036dec3e6a0fac84e167d9131242e0c
Diff: https://reviews.apache.org/r/19762/diff/
Testing
-------
mesos-tests.sh –verbose –gtest_filter=“RegistrarTest*” –gtest_also_run_disabled_tests
On test machines this reduces the time for applying 20,000 (re)admit operations from 30secs to < 100ms
Thanks,
Jiang Yan Xu
Re: Review Request 19762: Improved Registrar performance by using a hashset
on SlaveIDs.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19762/#review39804
-----------------------------------------------------------
Ship it!
Looks good to me, I included a comment below about how we may want to add a TODO to reflect that ultimately accumulators would need to be done a bit differently if we aim to make a generic 'batch operation' abstraction.
I would love to see some additional tests now that we're relying on the hashset to make decisions. Namely, have a test that uses a second Registrar to verify that the first Registrar left the 'Registry' protobuf in the correct state. Does that sound good? If so, please follow up in a subsequent change.
src/master/registrar.hpp
<https://reviews.apache.org/r/19762/#comment72412>
We should mention that we'd like to do this because it turns out this idea of "doing batches of operations" is likely to be used elsewhere in the code (potentially when we add repair state, offer reservations, etc).
src/master/registrar.hpp
<https://reviews.apache.org/r/19762/#comment72411>
Let's add to this comment to reflect that the operation is now performed on the 'registry' and the accumulators, which in this case is just 'slaveIDs'.
src/master/registrar.cpp
<https://reviews.apache.org/r/19762/#comment72436>
How about:
// Create the 'slaveIds' accumulator.
So rather than constructing the 'accumulator' (slaveIds) from 'T' (Registry) once the state has been fetched, we're only materializing accumulators for the purpose of applying operations. That seems fine but it would be good to include a comment that we're doing this because the logic is simpler, is that correct?
The alternative is to treat 'T' and the 'accumulators' in the same manner:
Option<Variable<Registry> > variable; // T.
Option<hashset<SlaveID> > slaveIds; // Accumulator 1.
_recover(...)
{
...
variable = recovery.get();
slaveIds = SlaveIDs(variable.get()); // Imagine a 'SlaveIDs' struct possibly extending from an
// 'Accumulator', accumulators would be construct-able from a 'T'.
}
And the code in update() copies 'Registry' and all accumulators in the same manner:
update(...)
{
// Copy 'T' and 'accumulators'.
Registry registry = variable.get().get();
SlaveIDs slaveIds = slaveIds;
...
}
_update(...)
{
// All accumulators get re-constructed from the new 'T'.
}
Of course, this does look to be more complex in that it involves less "local" reasoning, but it offers the opportunity for accumulators to be leveraged *across* batches. This would only be beneficial if copying the accumulator is cheaper than constructing a fresh one.
What you have now seems good to me, but let's at least add a TODO on 'Registrar' for including accumulators as template parameters.
- Ben Mahler
On April 3, 2014, 10:30 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19762/
> -----------------------------------------------------------
>
> (Updated April 3, 2014, 10:30 p.m.)
>
>
> Review request for mesos, Ben Mahler and Vinod Kone.
>
>
> Bugs: MESOS-1155
> https://issues.apache.org/jira/browse/MESOS-1155
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/master.hpp fef59c9971fcb3a5a33d7c94210b87edacb5719b
> src/master/registrar.hpp 0d659c57d55f7d054e106477c814d921479e3177
> src/master/registrar.cpp 9c0537d86036dec3e6a0fac84e167d9131242e0c
>
> Diff: https://reviews.apache.org/r/19762/diff/
>
>
> Testing
> -------
>
> mesos-tests.sh –verbose –gtest_filter=“RegistrarTest*” –gtest_also_run_disabled_tests
> On test machines this reduces the time for applying 20,000 (re)admit operations from 30secs to < 100ms
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 19762: Improved Registrar performance by using a hashset
on SlaveIDs.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19762/
-----------------------------------------------------------
(Updated April 3, 2014, 3:30 p.m.)
Review request for mesos, Ben Mahler and Vinod Kone.
Changes
-------
Vinod's comments. NNFR.
Bugs: MESOS-1155
https://issues.apache.org/jira/browse/MESOS-1155
Repository: mesos-git
Description
-------
See summary.
Diffs (updated)
-----
src/master/master.hpp fef59c9971fcb3a5a33d7c94210b87edacb5719b
src/master/registrar.hpp 0d659c57d55f7d054e106477c814d921479e3177
src/master/registrar.cpp 9c0537d86036dec3e6a0fac84e167d9131242e0c
Diff: https://reviews.apache.org/r/19762/diff/
Testing
-------
mesos-tests.sh –verbose –gtest_filter=“RegistrarTest*” –gtest_also_run_disabled_tests
On test machines this reduces the time for applying 20,000 (re)admit operations from 30secs to < 100ms
Thanks,
Jiang Yan Xu
Re: Review Request 19762: Improved Registrar performance by using a hashset
on SlaveIDs.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19762/#review39185
-----------------------------------------------------------
src/master/registrar.cpp
<https://reviews.apache.org/r/19762/#comment71561>
s/;/./
src/master/registrar.cpp
<https://reviews.apache.org/r/19762/#comment71543>
s/hashset/hashset of SlaveIDs/
- Vinod Kone
On April 1, 2014, 1:13 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19762/
> -----------------------------------------------------------
>
> (Updated April 1, 2014, 1:13 a.m.)
>
>
> Review request for mesos, Ben Mahler and Vinod Kone.
>
>
> Bugs: MESOS-1155
> https://issues.apache.org/jira/browse/MESOS-1155
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/master/master.hpp eb8cd73e9e8f26a7438070ca2bb078bd2d1ec480
> src/master/registrar.hpp 0d659c57d55f7d054e106477c814d921479e3177
> src/master/registrar.cpp 9c0537d86036dec3e6a0fac84e167d9131242e0c
>
> Diff: https://reviews.apache.org/r/19762/diff/
>
>
> Testing
> -------
>
> mesos-tests.sh –verbose –gtest_filter=“RegistrarTest*” –gtest_also_run_disabled_tests
> On test machines this reduces the time for applying 20,000 (re)admit operations from 30secs to < 100ms
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 19762: Improved Registrar performance by using a hashset
on SlaveIDs.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19762/
-----------------------------------------------------------
(Updated March 31, 2014, 6:13 p.m.)
Review request for mesos, Ben Mahler and Vinod Kone.
Bugs: MESOS-1155
https://issues.apache.org/jira/browse/MESOS-1155
Repository: mesos-git
Description
-------
See summary.
Diffs
-----
src/master/master.hpp eb8cd73e9e8f26a7438070ca2bb078bd2d1ec480
src/master/registrar.hpp 0d659c57d55f7d054e106477c814d921479e3177
src/master/registrar.cpp 9c0537d86036dec3e6a0fac84e167d9131242e0c
Diff: https://reviews.apache.org/r/19762/diff/
Testing
-------
mesos-tests.sh –verbose –gtest_filter=“RegistrarTest*” –gtest_also_run_disabled_tests
On test machines this reduces the time for applying 20,000 (re)admit operations from 30secs to < 100ms
Thanks,
Jiang Yan Xu