----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39018/#review105234 -----------------------------------------------------------
Ship it! Looks great! Just a couple of nits and test comment suggestions. Fix it, then we'll ship it! include/mesos/resources.hpp (lines 118 - 127) <https://reviews.apache.org/r/39018/#comment163733> Looks like different wrapping rules were used for these two paragraphs. We've recently changed the style guide so that comments also wrap at 80 chars now. Please be consistent in new/edited comments. include/mesos/resources.hpp (line 127) <https://reviews.apache.org/r/39018/#comment163731> s/a ranges/a range/? src/common/resources.cpp (lines 457 - 458) <https://reviews.apache.org/r/39018/#comment163738> We only indent 2 characters when continuing an assignment ('=') on a newline. See `grep -rn -A1 "=$" src/` src/common/resources.cpp (lines 471 - 472) <https://reviews.apache.org/r/39018/#comment163739> Why wrap here? Wasn't this previously 80 chars exactly? src/common/resources.cpp (lines 1109 - 1110) <https://reviews.apache.org/r/39018/#comment163741> Only need a single blank line here, since you're following a comment block, not another function. src/tests/resources_tests.cpp (lines 235 - 236) <https://reviews.apache.org/r/39018/#comment163746> A panda is more than just a number. They have names too. See https://en.wikipedia.org/wiki/List_of_giant_pandas src/tests/resources_tests.cpp (line 433) <https://reviews.apache.org/r/39018/#comment163751> Missing comma in Resource array. src/tests/resources_tests.cpp (lines 451 - 452) <https://reviews.apache.org/r/39018/#comment163750> Whatever happened to panda4? src/tests/resources_tests.cpp (line 460) <https://reviews.apache.org/r/39018/#comment163752> Missing comma in Set list. src/tests/resources_tests.cpp (lines 506 - 507) <https://reviews.apache.org/r/39018/#comment163753> That elusive panda4.. where did she go? src/tests/resources_tests.cpp (line 576) <https://reviews.apache.org/r/39018/#comment163754> Missing 'type' field. src/tests/resources_tests.cpp (line 609) <https://reviews.apache.org/r/39018/#comment163755> Unrecognized Resource 'type'. src/tests/resources_tests.cpp (line 665) <https://reviews.apache.org/r/39018/#comment163756> "mode : RW" (since that's the only mode we support) - Adam B On Nov. 4, 2015, 8:21 a.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39018/ > ----------------------------------------------------------- > > (Updated Nov. 4, 2015, 8:21 a.m.) > > > Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael > Park. > > > Bugs: MESOS-2467 > https://issues.apache.org/jira/browse/MESOS-2467 > > > Repository: mesos > > > Description > ------- > > This includes code changes necessary for JSON parsing of Resources. > Documentation changes will be posted soon in another review > (https://reviews.apache.org/r/39102/). > > > Diffs > ----- > > include/mesos/resources.hpp 6c3a065945eb56dc88df9c977e5ca11d4cbcbf61 > include/mesos/v1/resources.hpp fe8925ac851b74d1b37919f00afc7ed816f47ea5 > src/common/resources.cpp 601388c35a1bff37c58e753d1870d53b8d0af2d1 > src/tests/resources_tests.cpp 6584fc6c39e6ffe9f8085576677dcc669f127697 > src/v1/resources.cpp dc868903472f8f3a1ddc56092e3f8f81d953ce39 > > Diff: https://reviews.apache.org/r/39018/diff/ > > > Testing > ------- > > New code was added to `ResourcesTest.ParsingFromJSON`, and two new tests were > added: `ResourcesTest.ParsingFromJSONWithRoles` and > `ResourcesTest.ParsingFromJSONError`. These attempt to test common error > scenarios that might show up in user-supplied JSON. > > `make check` was used to confirm that all tests pass. > > The original resources parsing format is used throughout many other tests > (check `src/tests/sorter_tests.cpp`, for example), so it's clear that the > original parsing continues to work correctly. > > > Thanks, > > Greg Mann > >
