Mailing-List: contact mesos-dev-help@incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: mesos-dev@incubator.apache.org
Content-Type: multipart/alternative;
 boundary="===============2450580988252810409=="
MIME-Version: 1.0
Subject: Re: Review Request: Add whitelist slaves option to master
From: "Benjamin Hindman" <benh@berkeley.edu>
To: "Benjamin Hindman" <benh@berkeley.edu>
Cc: "Vinod Kone" <vinodkone@gmail.com>,
 "mesos" <mesos-dev@incubator.apache.org>
Date: Mon, 18 Jun 2012 20:54:40 -0000
Message-ID: <20120618205440.28904.47002@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Benjamin Hindman" <noreply@reviews.apache.org>
References: <20120618185305.28905.46388@reviews.apache.org>
In-Reply-To: <20120618185305.28905.46388@reviews.apache.org>
Reply-To: "Benjamin Hindman" <benh@berkeley.edu>

--===============2450580988252810409==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable


-----------------------------------------------------------
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<strin=
g>> for the type of whitelist instead. Having "no" whitelist (i.e., whiteli=
st.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 no=
w 'white-list'. Please pick one and be consistent (my vote is for 'whitelis=
t').


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

>


--===============2450580988252810409==--