> 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").
>
> Vinod Kone wrote:
> 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.
I'd like to keep a monolithic 'initialize'. It gets called exactly _once_ and
is very procedural in nature so it's helpful to be able to walk through it line
by line rather than jumping between functions. Moreover, as BenM points out,
moving code into sub-functions tends to increase complexity: (a) you have to
decide what should go where (which changes over time) and (b) if there are
dependencies it can get hard to reason about (i.e., does an instance variable
get set in function 'setupFoo' or in 'setupBar', because 'setupBaz' needs it
and gets invoked before 'setupFoo' but after 'setupBar', etc). In all other
circumstances we should keep functions small and "primitive" in nature.
In addition, any time we can write an idempotent/stateless function we should.
And we should always make these static methods or "top-level" functions
(preferring the latter if the functionality has nothing to do with the class).
The 'Flags' abstraction should really provide "programmatic defaults", but
since that functionality doesn't exist yet the code you moved to
'setupResources' ended up in
'initialize'. I'm all for moving this code out of 'initialize', but lets do it
by fixing stout Flags. I created an issue at 3rdparty/stout!
- Benjamin
-----------------------------------------------------------
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
>
>
