> 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.
>
> Benjamin Hindman wrote:
> 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!
>
> Vinod Kone wrote:
> I don't completely buy the "procedural" / "context switch" arguments
> here, because they can be argued both ways. For example, why do we need
> Framework::launch/destroyExecutor(), Executor::add/removeTask() which are not
> idempotent? Why not inline them wherever they are used? Every function
> involves a context switch. The tradeoff we need to consider is whether that
> context switch is "worth" it. For me the "worth" is ease of understanding and
> ability to modify later on.
>
> I personally like this
>
> initialize()
> {
> setupResources();
> setupRoutes();
> setupFoo();
> setupBar();
> }
>
> than
>
> initialize()
> {
> // 100 lines of something
> // 100 lines of something else
> // ....
> }
>
> The former tells me what this function is doing in general. If I am not
> sure what is happening in a sub-function (a sign that it is a badly designed
> function) then I can look into it. I remember that we talked about bloated
> functions before and agreed that any function exceeding ~40 lines needs
> refactoring. Now that doesn't mean a 400 line function should be chopped into
> 10 functions. It is likely that there is a bigger underlying problem.
>
> Regarding your points a) and b). a) Arent' we always making this
> decision. I don't think its that difficult to understand that a function
> called setupResources() sets up resources or a function called addTask() adds
> task to an internal structure. b) Again, isn't this always true. Isn't that
> why we have things like pre/post conditions.
>
> But perhaps, thats just me. Maybe setupResources() is not as "primitive"
> or "easy to understand" as I thought it is.
>
>
> Vinod Kone wrote:
> And yes, programmatic defaults for flags is a great idea. But that is
> kinda orthogonal to this discussion.
I don't think Framework::launch/destroyExecutor() and
Executor::add/removeTask() are analogous.
1. What does a Framework do? It launches and destroys executors. This is
functionality one expects and understands from the functions available.
2. Ditto for Executor.
3. setupResources is not functionality provided by the Slave, rather it's a
helper that was added from the fact that the flags don't have 'programmatic'
defaults. So, we had to write code to make reasonable estimates when the
particular resource was not present.
In your code snippets, you've constructed a bit of a strawman, if you change
your second example to:
initialize()
{
// Parse the resources from the flags.
...
// Install the message handlers.
...
}
It becomes a much more interesting comparison.
setupResources _can_ be an idempotent function, so we definitely would make it
such _if_ we were to add it, no? The only input state is the flags, and the
output is the Resources.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12010/#review22208
-----------------------------------------------------------
On June 21, 2013, 6:12 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12010/
> -----------------------------------------------------------
>
> (Updated June 21, 2013, 6:12 a.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
>
>
