mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ben Mahler" <benjamin.mah...@gmail.com>
Subject Re: Review Request 33376: MESOS-2633 Moved struct Framework methods to their own implementation class.
Date Thu, 30 Apr 2015 01:49:31 GMT


> On April 29, 2015, 8:24 p.m., Joris Van Remoortere wrote:
> >

I was surprised to see new functionality in this patch since the summary was code movement
:)

Just a quick note on DRY below.


> 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?

>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).


- Ben


-----------------------------------------------------------
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