mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Park" <mcyp...@gmail.com>
Subject Re: Review Request 39018: Added JSON parsing for Resources.
Date Wed, 14 Oct 2015 00:13:41 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message