----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33376/#review80894 -----------------------------------------------------------
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 :-) .gitignore-template <https://reviews.apache.org/r/33376/#comment131054> 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. include/mesos/type_utils.hpp <https://reviews.apache.org/r/33376/#comment131059> Can you check the style guide / other files for how we arrange includes? include/mesos/type_utils.hpp <https://reviews.apache.org/r/33376/#comment131056> I think we need an extra whitespace here. Can you check the style guide / other files? include/mesos/type_utils.hpp <https://reviews.apache.org/r/33376/#comment131055> This comment doesn't seem to convey anything about what this code does, rather why it's part of this review request. Let's make it something more meaningful. Also, comments end with a period. include/mesos/type_utils.hpp <https://reviews.apache.org/r/33376/#comment131058> Do we still need the `internal` if we're already in the namespace? Is this required to disambiguate? Maybe this could be part of the more useful comment above! include/mesos/type_utils.hpp <https://reviews.apache.org/r/33376/#comment131057> We need another newline here. Also, can you check the style for where we close other namespaces? We have a specific pattern than we use :-) src/Makefile.am <https://reviews.apache.org/r/33376/#comment131061> 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? src/master/framework.cpp <https://reviews.apache.org/r/33376/#comment131062> Can you check the style guide for includes here? src/master/framework.cpp <https://reviews.apache.org/r/33376/#comment131065> Can we split out any code changes into a seperate review (with a JIRA)? Here and elsewhere. See my high-level comment at the top of the review. src/master/master.hpp <https://reviews.apache.org/r/33376/#comment131064> We use a period at the end of comments. - Joris Van Remoortere On April 20, 2015, 10:38 p.m., Marco Massenzio wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33376/ > ----------------------------------------------------------- > > (Updated April 20, 2015, 10:38 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 > >
