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="===============6551435319531324184=="
MIME-Version: 1.0
Subject: Re: Review Request: Fixed slave to create work directory during
 initialization.
From: "Vinod Kone" <vinodkone@gmail.com>
To: "Jiang Yan Xu" <yan@jxu.me>, "Benjamin Hindman" <benh@berkeley.edu>,
 "Ben Mahler" <benjamin.mahler@gmail.com>
Cc: "mesos" <mesos-dev@incubator.apache.org>,
 "Vinod Kone" <vinodkone@gmail.com>
Date: Fri, 21 Jun 2013 00:19:26 -0000
Message-ID: <20130621001926.29623.50557@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Vinod Kone" <noreply@reviews.apache.org>
References: <20130620231612.29623.91245@reviews.apache.org>
In-Reply-To: <20130620231612.29623.91245@reviews.apache.org>
Reply-To: "Vinod Kone" <vinodkone@gmail.com>

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


> On June 20, 2013, 11:16 p.m., Ben Mahler wrote:
> > I see why you added the helper, but it seems odd that it's only needed =
/ used by initialization. I realize initialize is fairly large, but my pref=
erence would be to keep it in there. Alternatively, could the helper be sta=
tic, taking as input the flags, and returning a Resources object?
> =

> Vinod Kone wrote:
>     I really don't want to put it up in initialize(), because as you note=
d, its ~200 lines already! I think it reads pretty clear when you see it in=
 initialize(). Infact, I would even like all the installs to be refactored =
into something like setupRoutes().
>     =

>     Regarding a static helper, I don't understand the reasoning?
> =

> Ben Mahler wrote:
>     I guess this boils down to philosophy, I find it easier to read a lon=
g version of initialize() instead of jumping into helpers. With these helpe=
rs I would say they are especially confusing because they do not reduce com=
plexity, they merely pull code from a function into a sub-function (but tha=
t anyone can call!). Ultimately, this actually increases complexity of the =
code (same code, but more lines to express it, and I have to jump into diff=
erent contexts to understand initialize()).
>     =

>     The static helper would at least have the benefit of not modifying st=
ate in the slave, meaning one can clearly see how it is used ("given a Flag=
s object, this produces a Resources object and has no effect on the Slave b=
ecause it's a static function").

Yup, I think this is more philosophical than technical. For me the importan=
t thing is that a function has to do one thing and is easy to understand ba=
sed on its signature. And yes, I don't like reading functions that have 100=
s of lines. I tend to get lost trying to understand what is happening.


- Vinod


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12010/#review22208
-----------------------------------------------------------


On June 20, 2013, 11:02 p.m., Vinod Kone wrote:
> =

> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12010/
> -----------------------------------------------------------
> =

> (Updated June 20, 2013, 11:02 p.m.)
> =

> =

> Review request for mesos, Benjamin Hindman, Ben Mahler, and Jiang Yan Xu.
> =

> =

> Description
> -------
> =

> See summary
> =

> =

> Diffs
> -----
> =

>   src/slave/slave.hpp d1ba82ed0c87331c6c06b109bbc2b267a07cf335 =

>   src/slave/slave.cpp 952bd149247c2cc44b87d20fe63a45d011f5578b =

> =

> Diff: https://reviews.apache.org/r/12010/diff/
> =

> =

> Testing
> -------
> =

> make check
> =

> =

> Thanks,
> =

> Vinod Kone
> =

>


--===============6551435319531324184==--