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



src/common/resources.cpp (lines 268 - 271)
<https://reviews.apache.org/r/39018/#comment164101>

    Adam and others: please check out the changes here. Resources::validate() 
is called in the constructors for Resources, so I removed it from this function.



src/tests/resources_tests.cpp (lines 593 - 613)
<https://reviews.apache.org/r/39018/#comment164102>

    I fixed an unintended error in this jsonString, and then found that the 
parsing didn't return an error. It turns out that since Resources::validate() 
is called in the Resources constructor, this particular error gets silently 
ignored and the constructor returns an empty Resources object. This seems less 
than ideal to me, but as long as we want to continue error checking in the 
constructors, it's not clear to me exactly how we would propagate any errors 
produced there. In any case, with the existing constructors, the proper way to 
test this error is to check that we have an empty Resources object after 
parsing.


- Greg Mann


On Nov. 6, 2015, 6:08 p.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
> 
> (Updated Nov. 6, 2015, 6:08 p.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
> 
>

Reply via email to