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