You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Greg Mann <gr...@mesosphere.io> on 2015/10/05 23:36:46 UTC
Review Request 39018: Added JSON parsing for Resources.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/
-----------------------------------------------------------
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
-------
Added JSON parsing for Resources.
Diffs (updated)
-----
include/mesos/resources.hpp 6c3a065945eb56dc88df9c977e5ca11d4cbcbf61
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 (updated)
-------
`make check`
Thanks,
Greg Mann
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Oct. 6, 2015, 7:09 a.m., haosdent huang wrote:
> > src/common/resources.cpp, line 362
> > <https://reviews.apache.org/r/39018/diff/2/?file=1091293#file1091293line362>
> >
> > Is it would more clear to split three functions, one is for parse old form, one is for parse json object from and third one is for parse json array form.
Good idea, thanks haosdent!
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review101616
-----------------------------------------------------------
On Oct. 6, 2015, 11:02 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 6, 2015, 11:02 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.
>
>
> 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
> -------
>
> `make check`
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review101616
-----------------------------------------------------------
src/common/resources.cpp (line 362)
<https://reviews.apache.org/r/39018/#comment159054>
Is it would more clear to split three functions, one is for parse old form, one is for parse json object from and third one is for parse json array form.
- haosdent huang
On Oct. 5, 2015, 9:51 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 5, 2015, 9:51 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.
>
>
> 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
> -------
>
> `make check`
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review101589
-----------------------------------------------------------
include/mesos/resources.hpp (line 73)
<https://reviews.apache.org/r/39018/#comment159022>
s/name:value(role)/name(role):value
include/mesos/v1/resources.hpp (line 73)
<https://reviews.apache.org/r/39018/#comment159023>
s/name:value(role)/name(role):value
- Guangya Liu
On 十月 5, 2015, 9:51 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated 十月 5, 2015, 9:51 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.
>
>
> 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
> -------
>
> `make check`
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review101797
-----------------------------------------------------------
Ship it!
Ship It!
- haosdent huang
On Oct. 7, 2015, 3:24 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 7, 2015, 3:24 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.
>
>
> 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
> -------
>
> `make check`
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review101807
-----------------------------------------------------------
Patch looks great!
Reviews applied: [39018]
All tests passed.
- Mesos ReviewBot
On Oct. 7, 2015, 3:24 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 7, 2015, 3:24 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.
>
>
> 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
> -------
>
> `make check`
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Joseph Wu <jo...@mesosphere.io>.
> On Oct. 9, 2015, 4:14 p.m., Adam B wrote:
> > src/common/resources.cpp, lines 368-384
> > <https://reviews.apache.org/r/39018/diff/4/?file=1092358#file1092358line368>
> >
> > This smells like a hack to workaround a bug in picojson. Can you link to something that indicates this as best practice or a recommended workaround? Is there a picojson issue we can track to know if this ever gets fixed?
>
> Greg Mann wrote:
> I'll have to do some research on this one; there are no existing picojson issues addressing this (only 27 issues have ever been raised in the project :-), but I can certainly file one. I'll get back to you on this.
TLDR: Our bug, not a PicoJson bug. (Good catch though!)
We should have a check after parsing: https://github.com/apache/mesos/blob/master/3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp#L740
`picojson::parse` will return the current position of stream iterator. We should be saving this value and checking to see if it matches the end (minus whitespace).
See: https://github.com/kazuho/picojson/blob/25fc213cca61ea22b3c2e4db8def9927562ba5f7/picojson.h#L925
Note that the reason PicoJson does not barf on that input is because PicoJson supports "streaming" JSON objects. i.e. You can give it a "stream" like `{} {} {}`, and expect three objects if you parse three times (passing the iterator along).
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review102110
-----------------------------------------------------------
On Oct. 9, 2015, 5:43 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 9, 2015, 5:43 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.
>
>
> 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.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Oct. 9, 2015, 11:14 p.m., Adam B wrote:
> > include/mesos/resources.hpp, lines 83-98
> > <https://reviews.apache.org/r/39018/diff/4/?file=1092356#file1092356line83>
> >
> > Do these all need to be public?
Indeed they do not. In addition to making these private, I rearranged the member functions in resources.cpp so that private members are now located in an appropriately-labeled section.
> On Oct. 9, 2015, 11:14 p.m., Adam B wrote:
> > src/common/resources.cpp, lines 368-384
> > <https://reviews.apache.org/r/39018/diff/4/?file=1092358#file1092358line368>
> >
> > This smells like a hack to workaround a bug in picojson. Can you link to something that indicates this as best practice or a recommended workaround? Is there a picojson issue we can track to know if this ever gets fixed?
I'll have to do some research on this one; there are no existing picojson issues addressing this (only 27 issues have ever been raised in the project :-), but I can certainly file one. I'll get back to you on this.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review102110
-----------------------------------------------------------
On Oct. 10, 2015, 12:43 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 10, 2015, 12:43 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.
>
>
> 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.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Oct. 9, 2015, 11:14 p.m., Adam B wrote:
> > src/common/resources.cpp, lines 368-384
> > <https://reviews.apache.org/r/39018/diff/4/?file=1092358#file1092358line368>
> >
> > This smells like a hack to workaround a bug in picojson. Can you link to something that indicates this as best practice or a recommended workaround? Is there a picojson issue we can track to know if this ever gets fixed?
>
> Greg Mann wrote:
> I'll have to do some research on this one; there are no existing picojson issues addressing this (only 27 issues have ever been raised in the project :-), but I can certainly file one. I'll get back to you on this.
>
> Joseph Wu wrote:
> TLDR: Our bug, not a PicoJson bug. (Good catch though!)
>
> We should have a check after parsing: https://github.com/apache/mesos/blob/master/3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp#L740
>
> `picojson::parse` will return the current position of stream iterator. We should be saving this value and checking to see if it matches the end (minus whitespace).
> See: https://github.com/kazuho/picojson/blob/25fc213cca61ea22b3c2e4db8def9927562ba5f7/picojson.h#L925
>
> Note that the reason PicoJson does not barf on that input is because PicoJson supports "streaming" JSON objects. i.e. You can give it a "stream" like `{} {} {}`, and expect three objects if you parse three times (passing the iterator along).
Awesome, thanks for solving that Joseph! After looking through the picojson source, it seems like we could be using the two-argument `picojson::parse` that operates on a `std::string` (https://github.com/kazuho/picojson/blob/master/picojson.h#L938-L942), rather than the four-argument function we're calling.
In the `picojson::parse()` signatures that take iterators as input, it makes sense to return successfully if a valid JSON value is parsed, regardless of what comes afterward. However, in the two-argument `std::string` case it only makes sense for the input string to contain a single JSON value, and it seems to me that proper validation would return an error if non-whitespace trailing characters are found.
I filed a pull request for picojson (https://github.com/kazuho/picojson/pull/70), and if it's merged we could switch to the two-argument function call and forego any additional validation of our own. In the meantime, I opened a JIRA at https://issues.apache.org/jira/browse/MESOS-3698 to track the validation change, and I posted a review that adds this error checking into JSON::parse(), since one of the tests in this patch will not pass without it.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review102110
-----------------------------------------------------------
On Oct. 11, 2015, 2:09 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 11, 2015, 2:09 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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
>
> 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review102110
-----------------------------------------------------------
Looking good, but I haven't made it to the tests yet (coming soon), and I assume the v1 resources are identical?
Can you elaborate on the "Testing Done"? Call out any new/modified unit tests, or any existing tests that verify your changes did not affect functionality?
include/mesos/resources.hpp (line 80)
<https://reviews.apache.org/r/39018/#comment159629>
"a Resources object containing a single resource"? Looks like it's actually "a single Resource object". Maybe just s/Resources/Resource/?
include/mesos/resources.hpp (lines 83 - 98)
<https://reviews.apache.org/r/39018/#comment159678>
Do these all need to be public?
include/mesos/resources.hpp (line 98)
<https://reviews.apache.org/r/39018/#comment159639>
s/checkParsedResource/validateParsedResource/?
src/common/resources.cpp (line 362)
<https://reviews.apache.org/r/39018/#comment159681>
You call `resourcesTry.get()` 4 times in this block. Maybe worth a `JSON::Value resourcesValue = resourcesTry.get();` above?
src/common/resources.cpp (lines 368 - 384)
<https://reviews.apache.org/r/39018/#comment159684>
This smells like a hack to workaround a bug in picojson. Can you link to something that indicates this as best practice or a recommended workaround? Is there a picojson issue we can track to know if this ever gets fixed?
src/common/resources.cpp (line 482)
<https://reviews.apache.org/r/39018/#comment159696>
Only needed just before the `foreach`. Might be unnecessary if protobuf::parse errors, so let's move it down to where it's used.
src/common/resources.cpp (lines 501 - 508)
<https://reviews.apache.org/r/39018/#comment159692>
Duplicated code. Can you factor this out?
src/common/resources.cpp (line 522)
<https://reviews.apache.org/r/39018/#comment159697>
s/validation/validationError/ (or just `error`) because "validation.isSome()" sounds like a good thing.
src/common/resources.cpp (line 535)
<https://reviews.apache.org/r/39018/#comment159698>
Disk resources are fine. Just not a disk with a 'persistence' field.
src/common/resources.cpp (line 632)
<https://reviews.apache.org/r/39018/#comment159694>
No longer needs explicit namespace, since we're `using RepeatedPtrField`. Ditto for the 2 other uses in this file.
- Adam B
On Oct. 7, 2015, 8:24 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 7, 2015, 8:24 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.
>
>
> 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
> -------
>
> `make check`
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review102176
-----------------------------------------------------------
Patch looks great!
Reviews applied: [39018]
All tests passed.
- Mesos ReviewBot
On Oct. 10, 2015, 5:57 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 10, 2015, 5:57 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.
>
>
> 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.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review102294
-----------------------------------------------------------
Patch looks great!
Reviews applied: [39211, 39018]
All tests passed.
- Mesos ReviewBot
On Oct. 12, 2015, 6:38 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 12, 2015, 6:38 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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
>
> 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Guangya Liu <gy...@gmail.com>.
> On 十月 13, 2015, 5:38 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, line 367
> > <https://reviews.apache.org/r/39018/diff/10/?file=1096261#file1096261line367>
> >
> > Why not check error here?
>
> Greg Mann wrote:
> I'm sorry, I don't understand this comment. Are you referring to the error checking for trailing characters?
>
> Michael Park wrote:
> This is because the result is already of type `Try<Resources>`. No need to unwrap the content only to rewrap it into a `Try<Resources>`, we can just propagate whatever the result is.
Thanks Michael, got it.
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review102391
-----------------------------------------------------------
On 十月 13, 2015, 7:34 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated 十月 13, 2015, 7:34 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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
>
> 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Oct. 13, 2015, 5:38 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, line 367
> > <https://reviews.apache.org/r/39018/diff/10/?file=1096261#file1096261line367>
> >
> > Why not check error here?
I'm sorry, I don't understand this comment. Are you referring to the error checking for trailing characters?
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review102391
-----------------------------------------------------------
On Oct. 12, 2015, 6:38 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 12, 2015, 6:38 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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
>
> 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Michael Park <mc...@gmail.com>.
> On Oct. 13, 2015, 5:38 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, line 367
> > <https://reviews.apache.org/r/39018/diff/10/?file=1096261#file1096261line367>
> >
> > Why not check error here?
>
> Greg Mann wrote:
> I'm sorry, I don't understand this comment. Are you referring to the error checking for trailing characters?
This is because the result is already of type `Try<Resources>`. No need to unwrap the content only to rewrap it into a `Try<Resources>`, we can just propagate whatever the result is.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review102391
-----------------------------------------------------------
On Oct. 12, 2015, 6:38 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 12, 2015, 6:38 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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
>
> 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review102391
-----------------------------------------------------------
src/common/resources.cpp (line 367)
<https://reviews.apache.org/r/39018/#comment160024>
Why not check error here?
- Guangya Liu
On 十月 12, 2015, 6:38 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated 十月 12, 2015, 6:38 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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
>
> 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Michael Park <mc...@gmail.com>.
> On Oct. 13, 2015, 5 p.m., Michael Park wrote:
> > src/common/resources.cpp, line 361
> > <https://reviews.apache.org/r/39018/diff/10/?file=1096261#file1096261line361>
> >
> > I think it would be better to only support one way of doing things here. I would opt for the array format. Is supporting both formats explicitly a feature?
>
> Greg Mann wrote:
> No, it hasn't been explicitly called out as a feature. Why the desire to allow only one format? My thought was that any valid JSON string describing one or more Resource objects should return successfully, but I suppose there isn't too much need to accommodate a single resource object, since at the command line users will always be specifying multiple resources.
>
> Michael Park wrote:
> > Why the desire to allow only one format?
>
> (1) Generally speaking, it's good to limit the number of ways to do the same thing so that there's less to remember as the user
> (2) In this specific case, it can actually be confusing/ambiguous.
> That is, does this expect a single `Resource` object: `{ "name" : "cpus", ... }`, or
> does it expect a `Credentials`-like object where we specify something like: `{ "resources": { "name" : "cpus", ... }, { "name" : "disk", ... }, ... }`?
> (I know now that it does the first one, but at first glance I actually thought it does the second)
>
> Greg Mann wrote:
> Yea, I see your point with #1. Regarding #2, that is interesting... I went with a JSON array in this implementation due to the discussion on the JIRA ticket, but honestly I was unaware of the format of the `Credentials` JSON input. In order to make the formats more consistent, we could use the same form here: `{"resources": [{"name": "cpus", ...}, {"name": "disk"}, ...]}`. This would seem to be more in line with your point #1, keeping the interface simple for the user. Thoughts?
Yeah, except I think we're moving away from the `Credentials`-like JSON input. The `JSON::Array` parse feature was only recently introduced.
For example, Joseph initially introduced `MachineIDs` but got rid of it once the `JSON::Array` parse function was introduced. [r38011](https://reviews.apache.org/r/38011)
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review102443
-----------------------------------------------------------
On Oct. 14, 2015, 3:50 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 14, 2015, 3:50 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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
>
> 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Oct. 13, 2015, 5 p.m., Michael Park wrote:
> > src/common/resources.cpp, line 361
> > <https://reviews.apache.org/r/39018/diff/10/?file=1096261#file1096261line361>
> >
> > I think it would be better to only support one way of doing things here. I would opt for the array format. Is supporting both formats explicitly a feature?
>
> Greg Mann wrote:
> No, it hasn't been explicitly called out as a feature. Why the desire to allow only one format? My thought was that any valid JSON string describing one or more Resource objects should return successfully, but I suppose there isn't too much need to accommodate a single resource object, since at the command line users will always be specifying multiple resources.
>
> Michael Park wrote:
> > Why the desire to allow only one format?
>
> (1) Generally speaking, it's good to limit the number of ways to do the same thing so that there's less to remember as the user
> (2) In this specific case, it can actually be confusing/ambiguous.
> That is, does this expect a single `Resource` object: `{ "name" : "cpus", ... }`, or
> does it expect a `Credentials`-like object where we specify something like: `{ "resources": { "name" : "cpus", ... }, { "name" : "disk", ... }, ... }`?
> (I know now that it does the first one, but at first glance I actually thought it does the second)
Yea, I see your point with #1. Regarding #2, that is interesting... I went with a JSON array in this implementation due to the discussion on the JIRA ticket, but honestly I was unaware of the format of the `Credentials` JSON input. In order to make the formats more consistent, we could use the same form here: `{"resources": [{"name": "cpus", ...}, {"name": "disk"}, ...]}`. This would seem to be more in line with your point #1, keeping the interface simple for the user. Thoughts?
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review102443
-----------------------------------------------------------
On Oct. 13, 2015, 7:34 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 13, 2015, 7:34 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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
>
> 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Oct. 13, 2015, 5 p.m., Michael Park wrote:
> > src/common/resources.cpp, line 1198
> > <https://reviews.apache.org/r/39018/diff/10/?file=1096261#file1096261line1198>
> >
> > Why the move here?
>
> Greg Mann wrote:
> I was moving all of the private methods into an appropriately-labeled section. I think I will move some of the previous methods into the implementation as you suggested, so perhaps I will just return this instance of `find()` to its original location.
I went ahead and left `find()` and `_contains()` in this new "Private member functions" section, since it matches the organization of the rest of resources.cpp. This does move one of the `find()` implementations away from the other one; let me know if you think this is an issue.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review102443
-----------------------------------------------------------
On Oct. 14, 2015, 3:50 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 14, 2015, 3:50 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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
>
> 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Michael Park <mc...@gmail.com>.
> On Oct. 13, 2015, 5 p.m., Michael Park wrote:
> > src/common/resources.cpp, line 361
> > <https://reviews.apache.org/r/39018/diff/10/?file=1096261#file1096261line361>
> >
> > I think it would be better to only support one way of doing things here. I would opt for the array format. Is supporting both formats explicitly a feature?
>
> Greg Mann wrote:
> No, it hasn't been explicitly called out as a feature. Why the desire to allow only one format? My thought was that any valid JSON string describing one or more Resource objects should return successfully, but I suppose there isn't too much need to accommodate a single resource object, since at the command line users will always be specifying multiple resources.
> Why the desire to allow only one format?
(1) Generally speaking, it's good to limit the number of ways to do the same thing so that there's less to remember as the user
(2) In this specific case, it can actually be confusing/ambiguous.
That is, does this expect a single `Resource` object: `{ "name" : "cpus", ... }`, or
does it expect a `Credentials`-like object where we specify something like: `{ "resources": { "name" : "cpus", ... }, { "name" : "disk", ... }, ... }`?
(I know now that it does the first one, but at first glance I actually thought it does the second)
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review102443
-----------------------------------------------------------
On Oct. 13, 2015, 7:34 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 13, 2015, 7:34 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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
>
> 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Oct. 13, 2015, 5 p.m., Michael Park wrote:
> > src/common/resources.cpp, line 361
> > <https://reviews.apache.org/r/39018/diff/10/?file=1096261#file1096261line361>
> >
> > I think it would be better to only support one way of doing things here. I would opt for the array format. Is supporting both formats explicitly a feature?
No, it hasn't been explicitly called out as a feature. Why the desire to allow only one format? My thought was that any valid JSON string describing one or more Resource objects should return successfully, but I suppose there isn't too much need to accommodate a single resource object, since at the command line users will always be specifying multiple resources.
> On Oct. 13, 2015, 5 p.m., Michael Park wrote:
> > src/common/resources.cpp, line 1198
> > <https://reviews.apache.org/r/39018/diff/10/?file=1096261#file1096261line1198>
> >
> > Why the move here?
I was moving all of the private methods into an appropriately-labeled section. I think I will move some of the previous methods into the implementation as you suggested, so perhaps I will just return this instance of `find()` to its original location.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review102443
-----------------------------------------------------------
On Oct. 13, 2015, 7:34 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 13, 2015, 7:34 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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
>
> 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review102443
-----------------------------------------------------------
The documentation added in this patch is great! Also consider breaking it out to a separate patch to separate behavioral vs non-behavioral changes. It really helps whoever's reviewing to stay within a narrower context :)
include/mesos/resources.hpp (lines 69 - 70)
<https://reviews.apache.org/r/39018/#comment160111>
`s/name and role/name, value, and role/`
include/mesos/resources.hpp (lines 75 - 76)
<https://reviews.apache.org/r/39018/#comment160112>
`s/parsing is successful/parsing was successful/`
`s/an Error if it isn't/an Error otherwise/`
include/mesos/resources.hpp (lines 92 - 93)
<https://reviews.apache.org/r/39018/#comment160113>
Same comment as above.
include/mesos/v1/resources.hpp (lines 350 - 414)
<https://reviews.apache.org/r/39018/#comment160145>
Could you explain why these can't simply live in `resources.cpp` as implementation details? Do we actually refer to private parts of the `Resource` object within these functions?
src/common/resources.cpp (line 361)
<https://reviews.apache.org/r/39018/#comment160146>
I think it would be better to only support one way of doing things here. I would opt for the array format. Is supporting both formats explicitly a feature?
src/common/resources.cpp (lines 392 - 393)
<https://reviews.apache.org/r/39018/#comment160147>
Why the wrapping here?
src/common/resources.cpp (lines 515 - 516)
<https://reviews.apache.org/r/39018/#comment160148>
This fits in one line now.
src/common/resources.cpp (lines 601 - 602)
<https://reviews.apache.org/r/39018/#comment160149>
This fits in one line now.
src/common/resources.cpp (line 1156)
<https://reviews.apache.org/r/39018/#comment160153>
Why the move here?
- Michael Park
On Oct. 12, 2015, 6:38 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 12, 2015, 6:38 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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
>
> 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Oct. 13, 2015, 3:35 a.m., Guangya Liu wrote:
> > include/mesos/resources.hpp, lines 114-118
> > <https://reviews.apache.org/r/39018/diff/10/?file=1096259#file1096259line114>
> >
> > Can this be removed or merged to the under comments?
>
> Greg Mann wrote:
> I put the comment here so that it wouldn't interfere with the Doxygen documentation, which should be placed directly before the member declaration. I don't want to remove it, because I think the TODO is still valid. I'm open to moving it if there is a better place for it; do you have one in mind?
>
> Guangya Liu wrote:
> What about moving those comments to /**/ section?
>
> Greg Mann wrote:
> But then it will show up in the documentation, correct? My motivation for leaving it out of the /**/ section was to exclude this comment from the docs.
>
> Guangya Liu wrote:
> I think it is ok to show up some TODO in document, thoughts? Thanks.
Yea now that I think of it, the increased visibility of the TODO might even be a good thing :-) Moved.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review102382
-----------------------------------------------------------
On Oct. 14, 2015, 3:50 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 14, 2015, 3:50 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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
>
> 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Guangya Liu <gy...@gmail.com>.
> On 十月 13, 2015, 3:35 a.m., Guangya Liu wrote:
> > include/mesos/resources.hpp, lines 114-118
> > <https://reviews.apache.org/r/39018/diff/10/?file=1096259#file1096259line114>
> >
> > Can this be removed or merged to the under comments?
>
> Greg Mann wrote:
> I put the comment here so that it wouldn't interfere with the Doxygen documentation, which should be placed directly before the member declaration. I don't want to remove it, because I think the TODO is still valid. I'm open to moving it if there is a better place for it; do you have one in mind?
What about moving those comments to /**/ section?
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review102382
-----------------------------------------------------------
On 十月 13, 2015, 7:34 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated 十月 13, 2015, 7:34 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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
>
> 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Oct. 13, 2015, 3:35 a.m., Guangya Liu wrote:
> > include/mesos/resources.hpp, lines 114-118
> > <https://reviews.apache.org/r/39018/diff/10/?file=1096259#file1096259line114>
> >
> > Can this be removed or merged to the under comments?
>
> Greg Mann wrote:
> I put the comment here so that it wouldn't interfere with the Doxygen documentation, which should be placed directly before the member declaration. I don't want to remove it, because I think the TODO is still valid. I'm open to moving it if there is a better place for it; do you have one in mind?
>
> Guangya Liu wrote:
> What about moving those comments to /**/ section?
But then it will show up in the documentation, correct? My motivation for leaving it out of the /**/ section was to exclude this comment from the docs.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review102382
-----------------------------------------------------------
On Oct. 13, 2015, 7:34 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 13, 2015, 7:34 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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
>
> 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Oct. 13, 2015, 3:35 a.m., Guangya Liu wrote:
> > include/mesos/resources.hpp, lines 114-118
> > <https://reviews.apache.org/r/39018/diff/10/?file=1096259#file1096259line114>
> >
> > Can this be removed or merged to the under comments?
I put the comment here so that it wouldn't interfere with the Doxygen documentation, which should be placed directly before the member declaration. I don't want to remove it, because I think the TODO is still valid. I'm open to moving it if there is a better place for it; do you have one in mind?
> On Oct. 13, 2015, 3:35 a.m., Guangya Liu wrote:
> > include/mesos/resources.hpp, line 120
> > <https://reviews.apache.org/r/39018/diff/10/?file=1096259#file1096259line120>
> >
> > s/Validates/Validate
This description should be fine; see the Google C++ style guide, which states that function comments should be "descriptive" rather than "declarative": https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comments
> On Oct. 13, 2015, 3:35 a.m., Guangya Liu wrote:
> > include/mesos/resources.hpp, line 412
> > <https://reviews.apache.org/r/39018/diff/10/?file=1096259#file1096259line412>
> >
> > s/conflictingTypes/conflictTypes?
I would suggest either `conflictingTypes` or `typesConflict`, since these are both abbreviations of English sentences that describe what the method does, i.e. it "checks for conflicting types", or "checks to see if any types conflict". I don't like `conflictTypes` because it sounds like the method will cause a conflict of some kind. I like the current name, but am open to changing it if there is a better alternative.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review102382
-----------------------------------------------------------
On Oct. 12, 2015, 6:38 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 12, 2015, 6:38 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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
>
> 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Guangya Liu <gy...@gmail.com>.
> On 十月 13, 2015, 3:35 a.m., Guangya Liu wrote:
> > include/mesos/resources.hpp, lines 114-118
> > <https://reviews.apache.org/r/39018/diff/10/?file=1096259#file1096259line114>
> >
> > Can this be removed or merged to the under comments?
>
> Greg Mann wrote:
> I put the comment here so that it wouldn't interfere with the Doxygen documentation, which should be placed directly before the member declaration. I don't want to remove it, because I think the TODO is still valid. I'm open to moving it if there is a better place for it; do you have one in mind?
>
> Guangya Liu wrote:
> What about moving those comments to /**/ section?
>
> Greg Mann wrote:
> But then it will show up in the documentation, correct? My motivation for leaving it out of the /**/ section was to exclude this comment from the docs.
I think it is ok to show up some TODO in document, thoughts? Thanks.
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review102382
-----------------------------------------------------------
On 十月 13, 2015, 7:34 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated 十月 13, 2015, 7:34 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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
>
> 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review102382
-----------------------------------------------------------
include/mesos/resources.hpp (line 75)
<https://reviews.apache.org/r/39018/#comment160009>
s/Try/`Try`
include/mesos/resources.hpp (line 92)
<https://reviews.apache.org/r/39018/#comment160010>
s/Try/`Try`
include/mesos/resources.hpp (lines 113 - 117)
<https://reviews.apache.org/r/39018/#comment160013>
Can this be removed or merged to the under comments?
include/mesos/resources.hpp (line 119)
<https://reviews.apache.org/r/39018/#comment160011>
s/Validates/Validate
include/mesos/resources.hpp (line 121)
<https://reviews.apache.org/r/39018/#comment160012>
ditto
include/mesos/resources.hpp (line 357)
<https://reviews.apache.org/r/39018/#comment160014>
Can we add a simple example for a JSON Object resource in the comments? This can make the user more clear for how to define a JSON Object resource.
include/mesos/resources.hpp (line 360)
<https://reviews.apache.org/r/39018/#comment160016>
s/Try/`Try`
include/mesos/resources.hpp (line 374)
<https://reviews.apache.org/r/39018/#comment160015>
Can we add a simple example for a JSON Array resource in the comments? This can make the user more clear for how to define a JSON Array resource.
include/mesos/resources.hpp (line 377)
<https://reviews.apache.org/r/39018/#comment160017>
s/Try/`Try`
include/mesos/resources.hpp (line 393)
<https://reviews.apache.org/r/39018/#comment160018>
s/Option/`Option`
include/mesos/resources.hpp (line 411)
<https://reviews.apache.org/r/39018/#comment160020>
s/conflictingTypes/conflictTypes?
- Guangya Liu
On 十月 12, 2015, 6:38 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated 十月 12, 2015, 6:38 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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
>
> 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review102573
-----------------------------------------------------------
Patch looks great!
Reviews applied: [39211, 39018]
All tests passed.
- Mesos ReviewBot
On Oct. 13, 2015, 7:34 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 13, 2015, 7:34 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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
>
> 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review102584
-----------------------------------------------------------
include/mesos/resources.hpp (line 375)
<https://reviews.apache.org/r/39018/#comment160291>
Can you please show an example of input?
- Guangya Liu
On 十月 13, 2015, 7:34 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated 十月 13, 2015, 7:34 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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
>
> 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review103018
-----------------------------------------------------------
src/common/resources.cpp (line 274)
<https://reviews.apache.org/r/39018/#comment160864>
static?
src/common/resources.cpp (line 316)
<https://reviews.apache.org/r/39018/#comment160865>
static?
src/common/resources.cpp (line 348)
<https://reviews.apache.org/r/39018/#comment160866>
static
src/common/resources.cpp (line 363)
<https://reviews.apache.org/r/39018/#comment160863>
s/Value_Type/Value::Type
ditto as following
src/common/resources.cpp (line 488)
<https://reviews.apache.org/r/39018/#comment160867>
Shall we add the text into the log message to improve debug?
src/v1/resources.cpp (line 275)
<https://reviews.apache.org/r/39018/#comment160862>
static?
- Guangya Liu
On 十月 14, 2015, 3:50 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated 十月 14, 2015, 3:50 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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
>
> 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review102664
-----------------------------------------------------------
Patch looks great!
Reviews applied: [39211, 39018]
All tests passed.
- Mesos ReviewBot
On Oct. 14, 2015, 3:50 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 14, 2015, 3:50 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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
>
> 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Michael Park <mc...@gmail.com>.
> On Oct. 18, 2015, 6:18 p.m., Michael Park wrote:
> > src/common/resources.cpp, lines 482-532
> > <https://reviews.apache.org/r/39018/diff/13/?file=1098334#file1098334line482>
> >
> > How about a structure like the following?
> >
> > ```cpp
> > Resources result;
> >
> > // Populate `result` based on which format.
> > Try<JSON::Array> resourcesJSON = JSON::parse<JSON::Array>(text);
> > if (resourcesJSON.isSome()) {
> > result = internal::convertJSON(resourcesJSON.get(), defaultRole);
> > } else {
> > foreach (...) {
> > ...
> > result += resource.get();
> > }
> > }
> >
> > // Validate the result regardless of what format
> > Option<Error> validate = validateCommandLineResources(result);
> > if (validate.isSome()) {
> > return validate.get();
> > }
> >
> > return result;
> > ```
> >
> > `validateCommandLineResources` is as suggested before, with the `conflictingTypes` logic encapsulated within it.
> >
> > I noticed we only perform the minimal validation necessary for the semicolon-delimited string format currently.
> > That is, we only do the `conflictingTypes` check. While this is sufficient in the current state of the format,
> > I think it simplifies the code to just do the full check. It's also future-proof if we need to extend the string format later on.
>
> Greg Mann wrote:
> Yep I agree that this is cleaner. I implemented it and unfortunately, since `internal::convertJSON` returns a `Try<Resources>`, we have to unwrap the Try before assigning it to `result`. We could get around this by wrapping the `if ... else` block in a lambda and assigning the result to a `Try<Resources> result`. In that case, however, we end up wrapping the old-style parsed resources in a `Try` unnecessarily, so we do extra work either way. I think the current version (without the lambda) is a bit more readable, so I went with that one.
This is totally fine. It's no worse than what we do in the old-style resources where we unwrap each of the `Try<Resource>` to build the `result`:
```cpp
Try<Resource> resource = Resources::parse(name, pair[1], role);
if (resource.isError()) {
return Error(resource.error());
}
result += resource.get();
```
Pragramatically speaking, we also only do this on start-up for command-line parsing and maybe in tests. Not a big deal if we're not perf-optimized here.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review103063
-----------------------------------------------------------
On Oct. 20, 2015, 6:09 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 20, 2015, 6:09 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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
>
> 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Oct. 18, 2015, 6:18 p.m., Michael Park wrote:
> > src/common/resources.cpp, lines 482-532
> > <https://reviews.apache.org/r/39018/diff/13/?file=1098334#file1098334line482>
> >
> > How about a structure like the following?
> >
> > ```cpp
> > Resources result;
> >
> > // Populate `result` based on which format.
> > Try<JSON::Array> resourcesJSON = JSON::parse<JSON::Array>(text);
> > if (resourcesJSON.isSome()) {
> > result = internal::convertJSON(resourcesJSON.get(), defaultRole);
> > } else {
> > foreach (...) {
> > ...
> > result += resource.get();
> > }
> > }
> >
> > // Validate the result regardless of what format
> > Option<Error> validate = validateCommandLineResources(result);
> > if (validate.isSome()) {
> > return validate.get();
> > }
> >
> > return result;
> > ```
> >
> > `validateCommandLineResources` is as suggested before, with the `conflictingTypes` logic encapsulated within it.
> >
> > I noticed we only perform the minimal validation necessary for the semicolon-delimited string format currently.
> > That is, we only do the `conflictingTypes` check. While this is sufficient in the current state of the format,
> > I think it simplifies the code to just do the full check. It's also future-proof if we need to extend the string format later on.
Yep I agree that this is cleaner. I implemented it and unfortunately, since `internal::convertJSON` returns a `Try<Resources>`, we have to unwrap the Try before assigning it to `result`. We could get around this by wrapping the `if ... else` block in a lambda and assigning the result to a `Try<Resources> result`. In that case, however, we end up wrapping the old-style parsed resources in a `Try` unnecessarily, so we do extra work either way. I think the current version (without the lambda) is a bit more readable, so I went with that one.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review103063
-----------------------------------------------------------
On Oct. 20, 2015, 5:01 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 20, 2015, 5:01 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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
>
> 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review103063
-----------------------------------------------------------
src/common/resources.cpp (lines 482 - 530)
<https://reviews.apache.org/r/39018/#comment160924>
How about a structure like the following?
```cpp
Resources result;
// Populate `result` based on which format.
Try<JSON::Array> resourcesJSON = JSON::parse<JSON::Array>(text);
if (resourcesJSON.isSome()) {
result = internal::convertJSON(resourcesJSON.get(), defaultRole);
} else {
foreach (...) {
...
result += resource.get();
}
}
// Validate the result regardless of what format
Option<Error> validate = validateCommandLineResources(result);
if (validate.isSome()) {
return validate.get();
}
return result;
```
`validateCommandLineResources` is as suggested before, with the `conflictingTypes` logic encapsulated within it.
I noticed we only perform the minimal validation necessary for the semicolon-delimited string format currently.
That is, we only do the `conflictingTypes` check. While this is sufficient in the current state of the format,
I think it simplifies the code to just do the full check. It's also future-proof if we need to extend the string format later on.
- Michael Park
On Oct. 14, 2015, 3:50 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 14, 2015, 3:50 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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
>
> 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Oct. 18, 2015, 12:41 a.m., Michael Park wrote:
> > src/common/resources.cpp, line 377
> > <https://reviews.apache.org/r/39018/diff/13/?file=1098334#file1098334line377>
> >
> > I think you need to check for `resource.role() == "*"` instead, since `role` has `[default = "*"]`. Please verify.
The `has_field` methods return false if the default value was used to initialize the message; I checked this to be sure. This has the added benefit that the method doesn't need to hard-code the protobuf's default role.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review103042
-----------------------------------------------------------
On Oct. 20, 2015, 5:01 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 20, 2015, 5:01 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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
>
> 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Michael Park <mc...@gmail.com>.
> On Oct. 18, 2015, 12:41 a.m., Michael Park wrote:
> > src/common/resources.cpp, line 377
> > <https://reviews.apache.org/r/39018/diff/13/?file=1098334#file1098334line377>
> >
> > I think you need to check for `resource.role() == "*"` instead, since `role` has `[default = "*"]`. Please verify.
>
> Greg Mann wrote:
> The `has_field` methods return false if the default value was used to initialize the message; I checked this to be sure. This has the added benefit that the method doesn't need to hard-code the protobuf's default role.
Great! Thanks for checking.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review103042
-----------------------------------------------------------
On Oct. 20, 2015, 6:09 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 20, 2015, 6:09 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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
>
> 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review103042
-----------------------------------------------------------
src/common/resources.cpp (line 274)
<https://reviews.apache.org/r/39018/#comment160880>
`s/validateParsedResource/validateCommandLineResource/`?
src/common/resources.cpp (line 277)
<https://reviews.apache.org/r/39018/#comment160881>
Remove newline
src/common/resources.cpp (lines 280 - 281)
<https://reviews.apache.org/r/39018/#comment160882>
Might be better to print out the `stringify(resource)` rather than just the `resource.name()`?
src/common/resources.cpp (line 316)
<https://reviews.apache.org/r/39018/#comment160885>
The `nameTypes` parameter of this function is a in/out parameter. This pattern is generally discouraged, since reasoning about the state injected into this function as well as the non-local side-effects made within this function is difficult to reason about.
While it may be slightly less performant, I think we can make `validateParsedResource` be `validateParsedResources` and encapsulate this test within that function. What do you think?
src/common/resources.cpp (line 377)
<https://reviews.apache.org/r/39018/#comment160883>
I think you need to check for `resource.role() == "*"` instead, since `role` has `[default = "*"]`. Please verify.
src/common/resources.cpp (line 483)
<https://reviews.apache.org/r/39018/#comment160884>
Remove newline.
- Michael Park
On Oct. 14, 2015, 3:50 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 14, 2015, 3:50 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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
>
> 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review103071
-----------------------------------------------------------
include/mesos/resources.hpp (line 37)
<https://reviews.apache.org/r/39018/#comment160929>
I think we can remove this now that the parts that require it were moved to `src/common/resources.cpp`, right?
include/mesos/v1/resources.hpp (line 36)
<https://reviews.apache.org/r/39018/#comment160930>
I think we can remove this now that the parts that require it were moved to `src/v1/resources.cpp`, right?
- Michael Park
On Oct. 14, 2015, 3:50 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 14, 2015, 3:50 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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
>
> 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review104982
-----------------------------------------------------------
src/common/resources.cpp (lines 457 - 458)
<https://reviews.apache.org/r/39018/#comment163343>
Would be more familiar to use copy initialization syntax in this case:
```cpp
Try<Resources> resources =
internal::convertJSON(resourcesJSON.get(), defaultRole));
```
src/v1/resources.cpp (lines 458 - 459)
<https://reviews.apache.org/r/39018/#comment163345>
Same as above.
- Michael Park
On Oct. 20, 2015, 6:09 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 20, 2015, 6:09 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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
>
> 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Guangya Liu <gy...@gmail.com>.
> On 十一月 4, 2015, 2:54 a.m., Guangya Liu wrote:
> > src/v1/resources.cpp, line 1190
> > <https://reviews.apache.org/r/39018/diff/16/?file=1115282#file1115282line1190>
> >
> > remove this
>
> Greg Mann wrote:
> Rather than removing it, I thought it might be nice to expand this comment to make it more useful. Let me know what you think!
+1, thanks Greg.
- Guangya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review105030
-----------------------------------------------------------
On 十一月 4, 2015, 4:21 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated 十一月 4, 2015, 4:21 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Nov. 4, 2015, 2:54 a.m., Guangya Liu wrote:
> > src/v1/resources.cpp, line 1190
> > <https://reviews.apache.org/r/39018/diff/16/?file=1115282#file1115282line1190>
> >
> > remove this
Rather than removing it, I thought it might be nice to expand this comment to make it more useful. Let me know what you think!
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review105030
-----------------------------------------------------------
On Nov. 4, 2015, 3:31 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Nov. 4, 2015, 3:31 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review105030
-----------------------------------------------------------
src/v1/resources.cpp (line 1144)
<https://reviews.apache.org/r/39018/#comment163396>
remove this
- Guangya Liu
On 十一月 3, 2015, 10:55 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated 十一月 3, 2015, 10:55 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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
>
> 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Nov. 5, 2015, 1:33 p.m., Adam B wrote:
> > src/common/resources.cpp, lines 471-472
> > <https://reviews.apache.org/r/39018/diff/17/?file=1115706#file1115706line471>
> >
> > Why wrap here? Wasn't this previously 80 chars exactly?
It was 80 previously, but now we have 2 additional indent characters.
> On Nov. 5, 2015, 1:33 p.m., Adam B wrote:
> > src/common/resources.cpp, lines 457-458
> > <https://reviews.apache.org/r/39018/diff/17/?file=1115706#file1115706line457>
> >
> > We only indent 2 characters when continuing an assignment ('=') on a newline.
> > See `grep -rn -A1 "=$" src/`
Whoops sorry, introduced this one with my last set of changes.
> On Nov. 5, 2015, 1:33 p.m., Adam B wrote:
> > src/tests/resources_tests.cpp, lines 235-236
> > <https://reviews.apache.org/r/39018/diff/17/?file=1115707#file1115707line235>
> >
> > A panda is more than just a number. They have names too. See https://en.wikipedia.org/wiki/List_of_giant_pandas
I apologize to any pandas whom I may have offended. The test pandas have been given the names they truly deserve.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review105234
-----------------------------------------------------------
On Nov. 4, 2015, 4:21 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Nov. 4, 2015, 4:21 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review105208
-----------------------------------------------------------
Ship it!
Ship It!
- Guangya Liu
On 十一月 4, 2015, 4:21 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated 十一月 4, 2015, 4:21 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Nov. 8, 2015, 9:32 a.m., Adam B wrote:
> > src/common/resources.cpp, line 271
> > <https://reviews.apache.org/r/39018/diff/17-18/?file=1115706#file1115706line271>
> >
> > Red flag: "silently ignored" scares me. Don't you mean that an error in validate() will yield an empty Resource, which should have been checked before/after/during this call? Why can't we check Resources.isEmpty(Resource) here?
Sorry, yea I was tempted by the `Resources::Resources(RepeatedPtrField<Resource>)` constructor, which is perfect for this conversion but prevents us from catching any errors. I think in the long run it would be better to have a factory function for `Resources` which returns `Try<Resources`, but for now we can do some redundant error checking in `Resources::convertJSON()` which lets us catch errors properly. I created a JIRA for the factory function and mentioned it in a TODO. I also added to the tests a case with empty resources.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review105607
-----------------------------------------------------------
On Nov. 9, 2015, 5:39 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Nov. 9, 2015, 5:39 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review105607
-----------------------------------------------------------
Almost there, but I won't let you "silently ignore" any errors on my watch.
src/common/resources.cpp (line 271)
<https://reviews.apache.org/r/39018/#comment164232>
Red flag: "silently ignored" scares me. Don't you mean that an error in validate() will yield an empty Resource, which should have been checked before/after/during this call? Why can't we check Resources.isEmpty(Resource) here?
src/tests/resources_tests.cpp (line 603)
<https://reviews.apache.org/r/39018/#comment164233>
Ooh, that looks like the unintended JSON error you fixed.
- Adam B
On Nov. 6, 2015, 4:17 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, 4:17 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/
-----------------------------------------------------------
(Updated Nov. 9, 2015, 4:48 p.m.)
Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael Park.
Changes
-------
Catch error from `Resources::validate()`.
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 (updated)
-----
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
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Nov. 9, 2015, 6:43 a.m., Adam B wrote:
> > src/common/resources.cpp, lines 356-357
> > <https://reviews.apache.org/r/39018/diff/19-21/?file=1118742#file1118742line356>
> >
> > Doesn't Resources::validate() return an Error? Why not use that Error and its message with your return?
Derp. Fixed it, thanks Adam.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review105653
-----------------------------------------------------------
On Nov. 9, 2015, 4:48 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Nov. 9, 2015, 4:48 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review105653
-----------------------------------------------------------
Ship it!
Much better error handling, but we're still dropping an error message unnecessarily. Fix it, then I'll ship it.
src/common/resources.cpp (lines 353 - 354)
<https://reviews.apache.org/r/39018/#comment164264>
Doesn't Resources::validate() return an Error? Why not use that Error and its message with your return?
- Adam B
On Nov. 8, 2015, 9:57 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Nov. 8, 2015, 9:57 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/
-----------------------------------------------------------
(Updated Nov. 9, 2015, 5:57 a.m.)
Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael Park.
Changes
-------
Changed comments.
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 (updated)
-----
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
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/
-----------------------------------------------------------
(Updated Nov. 9, 2015, 5:39 a.m.)
Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael Park.
Changes
-------
Addressed comments.
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 (updated)
-----
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
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/
-----------------------------------------------------------
(Updated Nov. 7, 2015, 12:17 a.m.)
Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael Park.
Changes
-------
Rebase.
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 (updated)
-----
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
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Nov. 6, 2015, 6:13 p.m., Greg Mann wrote:
> >
Note: I raised the following issues just to call attention to these changes made in the patch. I think the code is ready to merge, but wanted to make sure these differences didn't go unnoticed.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review105486
-----------------------------------------------------------
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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Adam B <ad...@mesosphere.io>.
> On Nov. 6, 2015, 10:13 a.m., Greg Mann wrote:
> > src/tests/resources_tests.cpp, lines 593-613
> > <https://reviews.apache.org/r/39018/diff/18/?file=1118403#file1118403line593>
> >
> > 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.
> the proper way to test this error is to check that we have an empty Resources object after parsing.
Do it.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review105486
-----------------------------------------------------------
On Nov. 6, 2015, 4:17 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, 4:17 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
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.
Changes
-------
Addressed comments, removed Resources::validate() from Resources::validateCommandLineResources().
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 (updated)
-----
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
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/
-----------------------------------------------------------
(Updated Nov. 4, 2015, 4:21 p.m.)
Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael Park.
Changes
-------
Expanded comment in Resources::find().
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 (updated)
-----
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
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/
-----------------------------------------------------------
(Updated Nov. 4, 2015, 3:31 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 (updated)
-------
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
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/
-----------------------------------------------------------
(Updated Nov. 3, 2015, 10:55 p.m.)
Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael Park.
Changes
-------
Addressed comments.
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 (updated)
-----
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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
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
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review104985
-----------------------------------------------------------
Ship it!
Ship It!
- Michael Park
On Oct. 20, 2015, 6:09 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 20, 2015, 6:09 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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
>
> 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review103216
-----------------------------------------------------------
Ship it!
Thanks for the update!
- Guangya Liu
On Oct. 20, 2015, 6:09 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 20, 2015, 6:09 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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
>
> 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/
-----------------------------------------------------------
(Updated Oct. 20, 2015, 6:09 a.m.)
Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael Park.
Changes
-------
Fixed bug in validateCommandLineResources().
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 (updated)
-----
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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
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
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/
-----------------------------------------------------------
(Updated Oct. 20, 2015, 5:01 a.m.)
Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael Park.
Changes
-------
Formatting, inserted static keyword, refactored `validateCommandLineResources`, removed `conflictingTypes`.
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 (updated)
-----
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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
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
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/
-----------------------------------------------------------
(Updated Oct. 14, 2015, 3:50 p.m.)
Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael Park.
Changes
-------
Addressed comments: moved helper functions to resources.cpp, removed helper to convert JSON::Object to Resources.
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 (updated)
-----
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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
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
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/
-----------------------------------------------------------
(Updated Oct. 13, 2015, 7:34 p.m.)
Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael Park.
Changes
-------
Addressed comments.
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 (updated)
-----
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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
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
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review102512
-----------------------------------------------------------
Patch looks great!
Reviews applied: [39211, 39018]
All tests passed.
- Mesos ReviewBot
On Oct. 13, 2015, 6:30 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 13, 2015, 6:30 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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
>
> 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/
-----------------------------------------------------------
(Updated Oct. 13, 2015, 6:30 p.m.)
Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael Park.
Changes
-------
Addressed comments.
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 (updated)
-----
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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
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
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/
-----------------------------------------------------------
(Updated Oct. 12, 2015, 6:38 p.m.)
Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael Park.
Changes
-------
Removed periods from v1 error messages.
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 (updated)
-----
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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
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
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/
-----------------------------------------------------------
(Updated Oct. 12, 2015, 6:34 p.m.)
Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael Park.
Changes
-------
Removed periods from error messages.
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 (updated)
-----
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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
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
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/
-----------------------------------------------------------
(Updated Oct. 12, 2015, 6:24 p.m.)
Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael Park.
Changes
-------
Changed comments in resources.hpp to Doxygen format.
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 (updated)
-----
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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
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
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review102183
-----------------------------------------------------------
Patch looks great!
Reviews applied: [39211, 39018]
All tests passed.
- Mesos ReviewBot
On Oct. 11, 2015, 3:08 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 11, 2015, 3:08 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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
>
> 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Oct. 12, 2015, 7:48 a.m., Guangya Liu wrote:
> > include/mesos/resources.hpp, line 312
> > <https://reviews.apache.org/r/39018/diff/7/?file=1094565#file1094565line312>
> >
> > Can you please make those comments using https://github.com/apache/mesos/blob/master/docs/doxygen-style-guide.md ?
Good call! Done.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review102203
-----------------------------------------------------------
On Oct. 12, 2015, 6:24 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 12, 2015, 6:24 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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
>
> 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review102203
-----------------------------------------------------------
include/mesos/resources.hpp (line 312)
<https://reviews.apache.org/r/39018/#comment159764>
Can you please make those comments using https://github.com/apache/mesos/blob/master/docs/doxygen-style-guide.md ?
- Guangya Liu
On 十月 11, 2015, 3:08 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated 十月 11, 2015, 3:08 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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
>
> 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
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/
-----------------------------------------------------------
(Updated Oct. 11, 2015, 3:08 a.m.)
Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael Park.
Changes
-------
Added dependency.
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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
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
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/
-----------------------------------------------------------
(Updated Oct. 11, 2015, 2:09 a.m.)
Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael Park.
Changes
-------
Removed picojson::parse error checking.
Bugs: MESOS-2467
https://issues.apache.org/jira/browse/MESOS-2467
Repository: mesos
Description (updated)
-------
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 (updated)
-----
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 (updated)
-------
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, with one notable failure (ResourcesTest.ParsingFromJSONError) related to a picojson issue tracked here: https://issues.apache.org/jira/browse/MESOS-3698
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
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/
-----------------------------------------------------------
(Updated Oct. 10, 2015, 5:57 p.m.)
Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael Park.
Changes
-------
Adjusted indents in tests.
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.
Diffs (updated)
-----
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.
Thanks,
Greg Mann
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review102151
-----------------------------------------------------------
Patch looks great!
Reviews applied: [39018]
All tests passed.
- Mesos ReviewBot
On Oct. 10, 2015, 12:43 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 10, 2015, 12:43 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.
>
>
> 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.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/
-----------------------------------------------------------
(Updated Oct. 10, 2015, 12:43 a.m.)
Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael Park.
Changes
-------
Addressed comments.
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.
Diffs (updated)
-----
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 (updated)
-------
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.
Thanks,
Greg Mann
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review101871
-----------------------------------------------------------
Ship it!
Ship It!
- Guangya Liu
On 十月 7, 2015, 3:24 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated 十月 7, 2015, 3:24 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.
>
>
> 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
> -------
>
> `make check`
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/
-----------------------------------------------------------
(Updated Oct. 7, 2015, 3:24 p.m.)
Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael Park.
Changes
-------
Addressed comments.
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.
Diffs (updated)
-----
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
-------
`make check`
Thanks,
Greg Mann
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Oct. 7, 2015, 3:44 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, line 354
> > <https://reviews.apache.org/r/39018/diff/3/?file=1092090#file1092090line354>
> >
> > What does this mean? In my understanding, the resource falg should support both string and json format, why remove old resource flag format?
My original thought was that we would eventually deprecate the old resources flag format. However, having spent some time testing the JSON flags at the command line, the JSON format is a bit verbose for that purpose :-) I agree that it would be nice to keep compatibility with both formats, as the colon-delimited format is nice for quickly running the agent at the command line.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review101740
-----------------------------------------------------------
On Oct. 7, 2015, 3:24 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 7, 2015, 3:24 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.
>
>
> 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
> -------
>
> `make check`
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Guangya Liu <gy...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review101740
-----------------------------------------------------------
src/common/resources.cpp (line 354)
<https://reviews.apache.org/r/39018/#comment159210>
What does this mean? In my understanding, the resource falg should support both string and json format, why remove old resource flag format?
- Guangya Liu
On 十月 6, 2015, 11:02 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated 十月 6, 2015, 11:02 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.
>
>
> 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
> -------
>
> `make check`
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review101739
-----------------------------------------------------------
Patch looks great!
Reviews applied: [39018]
All tests passed.
- Mesos ReviewBot
On Oct. 6, 2015, 11:02 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 6, 2015, 11:02 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.
>
>
> 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
> -------
>
> `make check`
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/
-----------------------------------------------------------
(Updated Oct. 6, 2015, 11:02 p.m.)
Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael Park.
Changes
-------
Addressed comments.
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.
Diffs (updated)
-----
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
-------
`make check`
Thanks,
Greg Mann
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/#review101590
-----------------------------------------------------------
Patch looks great!
Reviews applied: [39018]
All tests passed.
- Mesos ReviewBot
On Oct. 5, 2015, 9:51 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39018/
> -----------------------------------------------------------
>
> (Updated Oct. 5, 2015, 9:51 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.
>
>
> 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
> -------
>
> `make check`
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/
-----------------------------------------------------------
(Updated Oct. 5, 2015, 9:51 p.m.)
Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael Park.
Changes
-------
Copied changes to v1/resources.hpp.
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.
Diffs (updated)
-----
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
-------
`make check`
Thanks,
Greg Mann
Re: Review Request 39018: Added JSON parsing for Resources.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39018/
-----------------------------------------------------------
(Updated Oct. 5, 2015, 9:38 p.m.)
Review request for mesos, Adam B, Alexander Rukletsov, Jie Yu, and Michael Park.
Changes
-------
Updated description.
Bugs: MESOS-2467
https://issues.apache.org/jira/browse/MESOS-2467
Repository: mesos
Description (updated)
-------
This includes code changes necessary for JSON parsing of Resources. Documentation changes will be posted soon in another review.
Diffs
-----
include/mesos/resources.hpp 6c3a065945eb56dc88df9c977e5ca11d4cbcbf61
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
-------
`make check`
Thanks,
Greg Mann