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/06/18 20:31:59 UTC
Review Request: Add whitelist slaves option to master
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5396/
-----------------------------------------------------------
Review request for mesos and Benjamin Hindman.
Description
-------
Adds the ability to startup a master with a list of whitelisted slaves to send offers for.
Diffs
-----
src/master/constants.hpp c3b0b25
src/master/master.hpp 886f79c
src/master/master.cpp 89cdaf6
src/master/simple_allocator.cpp 1c54feb
src/tests/master_tests.cpp fcaf7dc
src/tests/utils_tests.cpp 0e3374e
Diff: https://reviews.apache.org/r/5396/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Add whitelist slaves option to master
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5396/#review8411
-----------------------------------------------------------
Ship it!
Ship It!
- Benjamin Hindman
On June 19, 2012, 11:10 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5396/
> -----------------------------------------------------------
>
> (Updated June 19, 2012, 11:10 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Thomas Marshall.
>
>
> Description
> -------
>
> Adds the ability to startup a master with a list of whitelisted slaves to send offers for.
>
>
> This addresses bug mesos-208.
> https://issues.apache.org/jira/browse/mesos-208
>
>
> Diffs
> -----
>
> src/common/option.hpp 1ec2f7e
> src/master/allocator.hpp 0fc61a5
> src/master/constants.hpp c3b0b25
> src/master/master.hpp 886f79c
> src/master/master.cpp 89cdaf6
> src/master/simple_allocator.hpp 034b5d0
> src/master/simple_allocator.cpp 1c54feb
> src/tests/master_tests.cpp fcaf7dc
> src/tests/utils_tests.cpp 0e3374e
> third_party/libprocess/include/process/option.hpp 5c4b1a4
>
> Diff: https://reviews.apache.org/r/5396/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Add whitelist slaves option to master
Posted by John Sirois <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5396/#review8552
-----------------------------------------------------------
Ship it!
src/master/master.cpp
<https://reviews.apache.org/r/5396/#comment18215>
Kill this - its shadowed by utils::os::read below and unused afaict
- John Sirois
On June 20, 2012, 5:59 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5396/
> -----------------------------------------------------------
>
> (Updated June 20, 2012, 5:59 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Thomas Marshall.
>
>
> Description
> -------
>
> Adds the ability to startup a master with a list of whitelisted slaves to send offers for.
>
>
> This addresses bug mesos-208.
> https://issues.apache.org/jira/browse/mesos-208
>
>
> Diffs
> -----
>
> src/common/option.hpp 1ec2f7e
> src/master/allocator.hpp 0fc61a5
> src/master/constants.hpp c3b0b25
> src/master/master.hpp 886f79c
> src/master/master.cpp 89cdaf6
> src/master/simple_allocator.hpp 034b5d0
> src/master/simple_allocator.cpp 1c54feb
> src/tests/master_tests.cpp fcaf7dc
> src/tests/utils_tests.cpp 0e3374e
> third_party/libprocess/include/process/option.hpp 5c4b1a4
>
> Diff: https://reviews.apache.org/r/5396/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Add whitelist slaves option to master
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5396/
-----------------------------------------------------------
(Updated June 20, 2012, 5:59 p.m.)
Review request for mesos, Benjamin Hindman and Thomas Marshall.
Changes
-------
s/const int WATCH_TIMEOUT = 5.0/const int WATCH_TIMEOUT = 5/
Description
-------
Adds the ability to startup a master with a list of whitelisted slaves to send offers for.
This addresses bug mesos-208.
https://issues.apache.org/jira/browse/mesos-208
Diffs (updated)
-----
src/common/option.hpp 1ec2f7e
src/master/allocator.hpp 0fc61a5
src/master/constants.hpp c3b0b25
src/master/master.hpp 886f79c
src/master/master.cpp 89cdaf6
src/master/simple_allocator.hpp 034b5d0
src/master/simple_allocator.cpp 1c54feb
src/tests/master_tests.cpp fcaf7dc
src/tests/utils_tests.cpp 0e3374e
third_party/libprocess/include/process/option.hpp 5c4b1a4
Diff: https://reviews.apache.org/r/5396/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Add whitelist slaves option to master
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5396/
-----------------------------------------------------------
(Updated June 19, 2012, 11:10 p.m.)
Review request for mesos, Benjamin Hindman and Thomas Marshall.
Changes
-------
added thomas to the review
Description
-------
Adds the ability to startup a master with a list of whitelisted slaves to send offers for.
This addresses bug mesos-208.
https://issues.apache.org/jira/browse/mesos-208
Diffs
-----
src/common/option.hpp 1ec2f7e
src/master/allocator.hpp 0fc61a5
src/master/constants.hpp c3b0b25
src/master/master.hpp 886f79c
src/master/master.cpp 89cdaf6
src/master/simple_allocator.hpp 034b5d0
src/master/simple_allocator.cpp 1c54feb
src/tests/master_tests.cpp fcaf7dc
src/tests/utils_tests.cpp 0e3374e
third_party/libprocess/include/process/option.hpp 5c4b1a4
Diff: https://reviews.apache.org/r/5396/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Add whitelist slaves option to master
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5396/
-----------------------------------------------------------
(Updated June 19, 2012, 11:09 p.m.)
Review request for mesos and Benjamin Hindman.
Changes
-------
ben's comments
Description
-------
Adds the ability to startup a master with a list of whitelisted slaves to send offers for.
This addresses bug mesos-208.
https://issues.apache.org/jira/browse/mesos-208
Diffs (updated)
-----
src/common/option.hpp 1ec2f7e
src/master/allocator.hpp 0fc61a5
src/master/constants.hpp c3b0b25
src/master/master.hpp 886f79c
src/master/master.cpp 89cdaf6
src/master/simple_allocator.hpp 034b5d0
src/master/simple_allocator.cpp 1c54feb
src/tests/master_tests.cpp fcaf7dc
src/tests/utils_tests.cpp 0e3374e
third_party/libprocess/include/process/option.hpp 5c4b1a4
Diff: https://reviews.apache.org/r/5396/diff/
Testing
-------
make check
Thanks,
Vinod Kone
Re: Review Request: Add whitelist slaves option to master
Posted by Vinod Kone <vi...@gmail.com>.
> On June 18, 2012, 8:54 p.m., Benjamin Hindman wrote:
> > src/master/master.cpp, line 71
> > <https://reviews.apache.org/r/5396/diff/1/?file=111895#file111895line71>
> >
> > Newline before comment please.
done
> On June 18, 2012, 8:54 p.m., Benjamin Hindman wrote:
> > src/master/master.cpp, line 76
> > <https://reviews.apache.org/r/5396/diff/1/?file=111895#file111895line76>
> >
> > These seem like esoteric semantics. You should use Option<hashset<string>> for the type of whitelist instead. Having "no" whitelist (i.e., whitelist.isNone()) seems better than checking for whitelist.contains("*").
fixed
> On June 18, 2012, 8:54 p.m., Benjamin Hindman wrote:
> > src/master/master.cpp, line 80
> > <https://reviews.apache.org/r/5396/diff/1/?file=111895#file111895line80>
> >
> > Wrap line please. You can put the first '<<' on a newline.
done
> On June 18, 2012, 8:54 p.m., Benjamin Hindman wrote:
> > src/master/master.cpp, line 83
> > <https://reviews.apache.org/r/5396/diff/1/?file=111895#file111895line83>
> >
> > Use strlen here please. Also, fix spaces around '-'.
done
> On June 18, 2012, 8:54 p.m., Benjamin Hindman wrote:
> > src/master/master.cpp, line 88
> > <https://reviews.apache.org/r/5396/diff/1/?file=111895#file111895line88>
> >
> > Please put this on the previous line.
done
> On June 18, 2012, 8:54 p.m., Benjamin Hindman wrote:
> > src/master/master.cpp, line 92
> > <https://reviews.apache.org/r/5396/diff/1/?file=111895#file111895line92>
> >
> > const &
done
> On June 18, 2012, 8:54 p.m., Benjamin Hindman wrote:
> > src/master/master.cpp, lines 93-94
> > <https://reviews.apache.org/r/5396/diff/1/?file=111895#file111895line93>
> >
> > Indentation looks wrong. Also, you use 'whitelist', 'white list' and now 'white-list'. Please pick one and be consistent (my vote is for 'whitelist').
fixed
> On June 18, 2012, 8:54 p.m., Benjamin Hindman wrote:
> > src/master/master.cpp, line 299
> > <https://reviews.apache.org/r/5396/diff/1/?file=111895#file111895line299>
> >
> > If you don't support this yet, please don't mislead users.
done
> On June 18, 2012, 8:54 p.m., Benjamin Hindman wrote:
> > src/master/master.cpp, line 1685
> > <https://reviews.apache.org/r/5396/diff/1/?file=111895#file111895line1685>
> >
> > What about slaves that were previously on the whitelist but are not any longer? This seems to be the crux condition of a dynamic whitelist.
good catch! fixed
> On June 18, 2012, 8:54 p.m., Benjamin Hindman wrote:
> > src/master/master.cpp, line 1727
> > <https://reviews.apache.org/r/5396/diff/1/?file=111895#file111895line1727>
> >
> > Right, this is where it seems nicer to just do: if (whitelist.isSome() && whitelist.contains(slave->info.hostname()) {
done
> On June 18, 2012, 8:54 p.m., Benjamin Hindman wrote:
> > src/master/master.hpp, line 221
> > <https://reviews.apache.org/r/5396/diff/1/?file=111894#file111894line221>
> >
> > Code related to allocations is being moved into the allocator. Putting the whitelist (and related constructs) in the master is creating more work for the people that are working on that!
moved to allocator.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5396/#review8352
-----------------------------------------------------------
On June 18, 2012, 6:53 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5396/
> -----------------------------------------------------------
>
> (Updated June 18, 2012, 6:53 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> Adds the ability to startup a master with a list of whitelisted slaves to send offers for.
>
>
> This addresses bug mesos-208.
> https://issues.apache.org/jira/browse/mesos-208
>
>
> Diffs
> -----
>
> src/master/constants.hpp c3b0b25
> src/master/master.hpp 886f79c
> src/master/master.cpp 89cdaf6
> src/master/simple_allocator.cpp 1c54feb
> src/tests/master_tests.cpp fcaf7dc
> src/tests/utils_tests.cpp 0e3374e
>
> Diff: https://reviews.apache.org/r/5396/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Add whitelist slaves option to master
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5396/#review8352
-----------------------------------------------------------
src/master/master.hpp
<https://reviews.apache.org/r/5396/#comment18008>
Code related to allocations is being moved into the allocator. Putting the whitelist (and related constructs) in the master is creating more work for the people that are working on that!
src/master/master.cpp
<https://reviews.apache.org/r/5396/#comment18009>
Newline before comment please.
src/master/master.cpp
<https://reviews.apache.org/r/5396/#comment18011>
These seem like esoteric semantics. You should use Option<hashset<string>> for the type of whitelist instead. Having "no" whitelist (i.e., whitelist.isNone()) seems better than checking for whitelist.contains("*").
src/master/master.cpp
<https://reviews.apache.org/r/5396/#comment18012>
Wrap line please. You can put the first '<<' on a newline.
src/master/master.cpp
<https://reviews.apache.org/r/5396/#comment18013>
Use strlen here please. Also, fix spaces around '-'.
src/master/master.cpp
<https://reviews.apache.org/r/5396/#comment18014>
Please put this on the previous line.
src/master/master.cpp
<https://reviews.apache.org/r/5396/#comment18015>
const &
src/master/master.cpp
<https://reviews.apache.org/r/5396/#comment18016>
Indentation looks wrong. Also, you use 'whitelist', 'white list' and now 'white-list'. Please pick one and be consistent (my vote is for 'whitelist').
src/master/master.cpp
<https://reviews.apache.org/r/5396/#comment18017>
If you don't support this yet, please don't mislead users.
src/master/master.cpp
<https://reviews.apache.org/r/5396/#comment18018>
What about slaves that were previously on the whitelist but are not any longer? This seems to be the crux condition of a dynamic whitelist.
src/master/master.cpp
<https://reviews.apache.org/r/5396/#comment18019>
Right, this is where it seems nicer to just do: if (whitelist.isSome() && whitelist.contains(slave->info.hostname()) {
src/tests/master_tests.cpp
<https://reviews.apache.org/r/5396/#comment18020>
Newline please.
src/tests/master_tests.cpp
<https://reviews.apache.org/r/5396/#comment18021>
Why not const?
src/tests/master_tests.cpp
<https://reviews.apache.org/r/5396/#comment18022>
I'd like it if we didn't introduce possible assertion violations in our tests.
src/tests/master_tests.cpp
<https://reviews.apache.org/r/5396/#comment18023>
Spaces.
- Benjamin Hindman
On June 18, 2012, 6:53 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5396/
> -----------------------------------------------------------
>
> (Updated June 18, 2012, 6:53 p.m.)
>
>
> Review request for mesos and Benjamin Hindman.
>
>
> Description
> -------
>
> Adds the ability to startup a master with a list of whitelisted slaves to send offers for.
>
>
> This addresses bug mesos-208.
> https://issues.apache.org/jira/browse/mesos-208
>
>
> Diffs
> -----
>
> src/master/constants.hpp c3b0b25
> src/master/master.hpp 886f79c
> src/master/master.cpp 89cdaf6
> src/master/simple_allocator.cpp 1c54feb
> src/tests/master_tests.cpp fcaf7dc
> src/tests/utils_tests.cpp 0e3374e
>
> Diff: https://reviews.apache.org/r/5396/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request: Add whitelist slaves option to master
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5396/
-----------------------------------------------------------
(Updated June 18, 2012, 6:53 p.m.)
Review request for mesos and Benjamin Hindman.
Changes
-------
attached bug
Description
-------
Adds the ability to startup a master with a list of whitelisted slaves to send offers for.
This addresses bug mesos-208.
https://issues.apache.org/jira/browse/mesos-208
Diffs
-----
src/master/constants.hpp c3b0b25
src/master/master.hpp 886f79c
src/master/master.cpp 89cdaf6
src/master/simple_allocator.cpp 1c54feb
src/tests/master_tests.cpp fcaf7dc
src/tests/utils_tests.cpp 0e3374e
Diff: https://reviews.apache.org/r/5396/diff/
Testing
-------
make check
Thanks,
Vinod Kone