mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marco Massenzio" <ma...@mesosphere.io>
Subject Re: Review Request 33376: MESOS-2633 Moved struct Framework methods to their own implementation class.
Date Thu, 30 Apr 2015 15:42:19 GMT


> On April 29, 2015, 8:24 p.m., Joris Van Remoortere wrote:
> > src/master/framework.cpp, line 187
> > <https://reviews.apache.org/r/33376/diff/3/?file=939881#file939881line187>
> >
> >     1) Let's sync with BenH if we want to factor out logging like this. There are
arguments on both sides, so let's follow up with a discussion, and maybe add it to the style
guide!
> >     2) *If* we *do* decide to factor out the logging like this, let's change the
name to be more meaninful, or even move this function into `updateFrameworkInfo` as a lambda.
This message is clearly specific to that function, and is not usable by other functions in
`Framework`.
> >     3) Since we're in the implementation file, we can do `using std::string` and
remove the namespacing of `std::string` here.
> 
> Marco Massenzio wrote:
>     I actually wasn't quite sure myself (and the more I think about this, the less I
like my own choice): this is just a helper function, to avoid violating the DRY Principle.
>     I would like to move it out of the Framework class, and just leave it the cpp file
as a local (static) helper method.
>     Again, to me, it scratches the itch of copy & pasted code, which I positively
dislike.
>     
>     If I change it to a local helper method, would that address your concerns?
> 
> Ben Mahler wrote:
>     >this is just a helper function, to avoid violating the DRY Principle.
>     
>     This is a great example of how to apply DRY! :)
>     
>     Namely, we have to be careful about introducing helper functions to follow the DRY
principle as it often comes at a cost: loss of readability. In this case, the reader encountering
`logWarning` has no _local_ clues to know that `logWarning` is not a general way to log warnings,
but rather it prints a special message about updating framework information. That doesn't
seem very intuitive? Also, now we have a function at the Framework scope, callable from any
other place in the framework code, which is going to mean that when a reader encounters it,
he/she won't be sure that it's only used within the updating FrameworkInfo code. So in the
process of applying DRY we've made our software harder to understand. Does this make sense?
>     
>     There are ways to follow DRY without sacrificing readability, for example, can we
collect the set of fields that we weren't able to change and just print them together in one
log message? If there's some reason to have multiple log messages, could we have a local lambda
for it? (Although I don't see a need for having 1 log message per field).

Absolutely, Ben, thanks for clarifying.
That's actually what I meant by 'scratchin the itch' (I realize now I was being economical
with words... I was typing single-handed... which is != from single-handedly typing :)
I've already reverted the code.


- Marco


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


On April 22, 2015, 8:35 p.m., Marco Massenzio wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33376/
> -----------------------------------------------------------
> 
> (Updated April 22, 2015, 8:35 p.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-2633
>     https://issues.apache.org/jira/browse/MESOS-2633
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Created new file framework.cpp containing all the methods' implementations for the `Framework`
class (declared in master/master.hpp)
> 
> Declared `operator ==` for Task in type_utils.hpp 
> (it was implemented before, but not declared in the header file);
> 
> Refactored all the LOG(WARNING) to a single utility method.
> 
> 
> Diffs
> -----
> 
>   include/mesos/type_utils.hpp cdf5864389a72002b538c263d70bcade2bdffa45 
>   src/Makefile.am 93c7c8a807a33ab639be6289535bbd32022aa85b 
>   src/master/framework.cpp PRE-CREATION 
>   src/master/master.hpp 550d2c50cd01aa5830a7e7e03ab4a0240c221b15 
> 
> Diff: https://reviews.apache.org/r/33376/diff/
> 
> 
> Testing
> -------
> 
> All tests (make check) pass.
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>


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