mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Vinod Kone" <vinodk...@gmail.com>
Subject Re: Review Request: Fixed slave to create work directory during initialization.
Date Fri, 21 Jun 2013 00:19:26 GMT


> 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 preference would be to keep it
in there. Alternatively, could the helper be static, 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 noted, 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 long version of
initialize() instead of jumping into helpers. With these helpers I would say they are especially
confusing because they do not reduce complexity, they merely pull code from a function into
a sub-function (but that 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 different contexts
to understand initialize()).
>     
>     The static helper would at least have the benefit of not modifying state in the slave,
meaning one can clearly see how it is used ("given a Flags object, this produces a Resources
object and has no effect on the Slave because it's a static function").

Yup, I think this is more philosophical than technical. For me the important thing is that
a function has to do one thing and is easy to understand based on its signature. And yes,
I don't like reading functions that have 100s 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message