----------------------------------------------------------- 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 > >
