> On April 21, 2015, 3:27 a.m., Joris Van Remoortere wrote: > > Hey Marco, great first patch! > > Some high level comments: > > - Can you check your editor settings to make sure you don't modify lines > > you don't intend? > > - Just do a quick look over the 'diff' section before publishing a review > > so can you see if everything being reviewed is intended (i.e. eclipse > > template) > > - If we're moving around large blocks of code, let's split out any > > refactoring / semantic changes into a seperate review. This way it's easier > > to see just which lines you actually modified vs. moved. It will get the > > code commited faster. (i.e. logging refactor). > > > > Can you make these changes and then I'll take a deeper look at > > framework.cpp :-) > > Marco Massenzio wrote: > 1. apart from removing tabs (which follows the "Boy Scout Principle") my > settings are just as you mention: I don't think my changes touched anything > other than intended changes? > The only areas I touched that were outside the scope was lining up the > continuation slashes (`\ `) on the .am file (which was, honestly, pretty > awful to look at :) > 2. I did and it seemed to me to be ok? (Eclipse 'ignores' ought to be > part of the .gitignore IMO as other developers who may be using Eclips will > need it? why not?) > 3. agreed - although it seemed to me such a minor non-functional change > that it did not warrant its own review: and the original code IMO should have > not gone past review anyway (violating DRY principle: so, as this was > technically "new" code of mine, I'd be on the 'blame' hook for it) > > I will revert the check on the non-contains for the task, that is a > 'material' change and can go in its own review. > Please advise on the .gitignore changes and why those should not be part > of the .gitignore.
1. I think you might have a different whitespace setting in eclipse. For everyone else the continuation slashes () in the .am file were lined up, and after your change will not be. We can sync on this together offline using vim or some other editor to verify this. 2. I think adding your eclipse ignore file is great for future developers. I'm just suggesting that we move it into a separate JIRA / review. This is just the way our project works. We try to have highly-specific, atomic reviews so that people can look through the commit log and see what they expect. 3. a) I wouldn't worry about having too many reviews, rather too few. Isolating the changes from the moves makes it easier for someone to review. Since the reviewer is also on the hook for the code, it's helpful to make it as easy as possibly on them to verify correctness. b) I totally understand your comment regarding the DRY principle. In the Mesos project sometimes we prefer to repeat ourselves, it's a style choice around ease of debugging. This is another discussion we can have offline as a team. c) Re: DRY, in this case the refactoring looks beneficial in my case, I would just suggest we make it a separate commit so that the cut between modification and movement of code is as clean as possible! Thanks for reverting the 'material' change! That way we can have a separate discussion regarding the behavior change and why we may want to follow that pattern here, and elsewhere. > On April 21, 2015, 3:27 a.m., Joris Van Remoortere wrote: > > src/Makefile.am, lines 302-304 > > <https://reviews.apache.org/r/33376/diff/1/?file=937065#file937065line302> > > > > I don't think you meant to change the spacing of all these lines > > (highlighted and rest of file). Can you check your editor settings to see > > if you're modifying them accidentally? > > Marco Massenzio wrote: > the right-hand side continuation \ was all over the place and looked > honestly awful: I follow the "boyscout principle" and just lined up quite a > few of them around the *one* change I made (adding `framework.cpp`) - > hopefully this should upset no one? I think we might have different whitespace modifiers in our editors. Let's sync up as in my high-level comment. > On April 21, 2015, 3:27 a.m., Joris Van Remoortere wrote: > > include/mesos/type_utils.hpp, line 184 > > <https://reviews.apache.org/r/33376/diff/1/?file=937064#file937064line184> > > > > I think we need an extra whitespace here. Can you check the style guide > > / other files? > > Marco Massenzio wrote: > I'm actually guessing you meant around the `==` in `operator == (...)`? > (you commented a blank line) > You're right - fixed Also a blank line. We have 2 blank lines between function implementations. > On April 21, 2015, 3:27 a.m., Joris Van Remoortere wrote: > > .gitignore-template, lines 22-33 > > <https://reviews.apache.org/r/33376/diff/1/?file=937063#file937063line22> > > > > I think this got added to your commit accidentaly? If you want to add > > this to the repository we should make a separate JIRA and review for this. > > Marco Massenzio wrote: > it was not accidental - but, really: do you need a review to add ignore > files? it seems a bit overkill to me. See my high-level comment. - Joris ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33376/#review80894 ----------------------------------------------------------- On April 21, 2015, 7:06 p.m., Marco Massenzio wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33376/ > ----------------------------------------------------------- > > (Updated April 21, 2015, 7:06 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 > ----- > > .gitignore-template 1a8e2a8677a29f06ba6386d56537a0013d38821c > include/mesos/type_utils.hpp cdf5864389a72002b538c263d70bcade2bdffa45 > src/Makefile.am d15a37365bcdd5c3906160b46b389635b38b1673 > src/master/framework.cpp PRE-CREATION > src/master/master.hpp c10e7c08c191acef9d31d98018a47a2a952a4dfc > > Diff: https://reviews.apache.org/r/33376/diff/ > > > Testing > ------- > > All tests (make check) pass. > > > Thanks, > > Marco Massenzio > >
