mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anindya Sinha <anindya_si...@apple.com>
Subject Re: Review Request 51999: Refactor parsing of resources.
Date Wed, 28 Sep 2016 19:24:35 GMT


> On Sept. 27, 2016, 5:28 p.m., Jiang Yan Xu wrote:
> > src/common/resources.cpp, lines 478-484
> > <https://reviews.apache.org/r/51999/diff/5/?file=1510283#file1510283line478>
> >
> >     Per our discussion I thought we would delay the validation until when we construct
`Resources` objects? (i.e., kill this).
> >     
> >     The result is that the two more primitive parsing functions `fromJSONString()`
and `fromSimpleString()` only captures syntactic errors (malformatted strings) but not semantic
errors. We can then take advantage of this in the later patch to fill in missing values to
make them semantically valid.

This was done in https://reviews.apache.org/r/51879/ when I added support for auto detection
since that is when we could not longer do a `validate()` at this point.


> On Sept. 27, 2016, 5:28 p.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, line 199
> > <https://reviews.apache.org/r/51999/diff/5/?file=1510281#file1510281line199>
> >
> >     s/JSON::Array& resourcesJSON/std::string text/
> >     
> >     Looking the current and potential call-sites, it doesn't look like we ever directly
call this method with a `JSON::Array` not parsed from text.
> >     
> >     Considering this, I think it would be more consistent to have the following
three methods which take the same argument list.
> >     
> >     ```
> >     static Try<Resources> parse(
> >         const std::string& text,
> >         const std::string& defaultRole = "*");
> >     
> >     static Try<std::vector<Resource>> fromJSONString(
> >         const std::string& text,
> >         const std::string& defaultRole = "*");
> >     
> >     static Try<vector<Resource>> fromSimpleString(
> >         const std::string& text,
> >         const std::string& defaultRole = "*")
> >     ```

For JSON handling, we do the following in sequence:
(1) `JSON::parse<JSON::Array>()` converts input string to JSON array. 
(2) `protobuf::parse<RepeatedPtrField<Resource>>(resourcesJSON)` converts the
JSON array to protobuf.

If we call both of these in `fromJSONString()`, then in `Resources::parse()`, we would not
be able to deduce which of the above 2 functions failed. This is needed since if it failed
in (1), it means we parse assuming text; whereas if it failed in (2), that is an error since
protobuf conversion failed for the JSON (we should not assume it is text).

So, I think this should work:
In `Resources::parse()`:
- Convert to JSON array (`JSON::parse<JSON::Array>()`).
- If `JSON::parse<JSON::Array>()` succeeds, process JSON array in `fromJSONString()`.
Any failure here is an error condition.
- If `JSON::parse<JSON::Array>()` fails, assume it is text and process in `fromSimpleString()`.


> On Sept. 27, 2016, 5:28 p.m., Jiang Yan Xu wrote:
> > src/common/resources.cpp, lines 587-617
> > <https://reviews.apache.org/r/51999/diff/5/?file=1510283#file1510283line587>
> >
> >     Is the following cleaner? (I added the logic to validate resources in the result
vector instead of implicit conversion)
> >     
> >     ```
> >     Try<vector<Resource>> resources = fromJSONString(text, defaultRole);
> >     
> >     // Parsing as a simple string if parsing as a JSON string fails.
> >     if (resources.isError()) {
> >       resources = fromSimpleString(text, defaultRole);
> >     }
> >     
> >     if (resources.isError()) {
> >       return Error(resources.error());
> >     }
> >     
> >     Resources result;
> >     
> >     foreach (const Resource& resource, resoruces) {
> >       // If invalid, propgate error instead of skipping the resource.
> >       Option<Error> error = Resources::validate(resource);
> >       
> >       if (error.isSome()) {
> >         return error.get();
> >       }
> >       
> >       result += resource;
> >     }
> >     
> >     Option<Error> error = internal::validateCommandLineResources(result);
> >     if (error.isSome()) {
> >       return error.get();
> >     }
> >     
> >     return result;
> >     ```
> 
> Guangya Liu wrote:
>     1) Can we kill the validation? Seems we already done the validation in each sub function.
>     
>     ```
>     // If invalid, propgate error instead of skipping the resource.
>     Option<Error> error = Resources::validate(resource);
>     ```
>     
>     2) Using `result.add(resource)` instead.

@xujyan: Please refer to my earlier response as to why we need to convert to JSON in `Resources::parse()`
and handle the JSON in `fromJSONString()` if JSON conversion is successful [If conversion
to protobuf fails in `fromJSONString()`, that is an error]; and assume it is text and process
in `fromSimpleString()` if JSON conversion fails.

@gyliu: 
(1) This patch still has the validation in individual functions so validation in `Resources::parse()`
is not needed but in  https://reviews.apache.org/r/51879/, we move the validation out of the
individual functions and consolidate into `Resources::parse()` [since we can have `Resource`
with no value].
(2) Using `result.add()` now.


> On Sept. 27, 2016, 5:28 p.m., Jiang Yan Xu wrote:
> > src/tests/resources_tests.cpp, lines 630-657
> > <https://reviews.apache.org/r/51999/diff/5/?file=1510284#file1510284line630>
> >
> >     With out refactor, the result of `Resources::parse()` doesn't change right?
> 
> Jiang Yan Xu wrote:
>     s/out/our/

Im original code: If a resource in text format is encountered with value of 0, it is dropped;
but in JSON, it flags an error. This patch makes the behavior same, ie. just drop if a `Resource`
has a value of 0. Hence, this test would fail for value of 0. So I modified this test to test
for negative values which is obviously an Error.


> On Sept. 27, 2016, 5:28 p.m., Jiang Yan Xu wrote:
> > include/mesos/resources.hpp, lines 169-172
> > <https://reviews.apache.org/r/51999/diff/5/?file=1510281#file1510281line169>
> >
> >     Now that we've moved these words to avoid duplication, we'd better refer to
the place this is moved to.
> >     
> >     ```
> >       /**
> >        * Parses Resources from text in the form of a JSON array. If that fails,

> >        * text in the form "name(role):value;name:value;...". i.e, this method
> >        * calls `fromJSONString()` or `fromSimpleString()` (see their documentation
> >        * for for details) and validates the result before converting it into a 
> >        * Resources object.
> >        */
> >     ```

Added the last comment regarding "validates the result before converting it into a Resources
object" in https://reviews.apache.org/r/51879/ because that is where we made that change.


- Anindya


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


On Sept. 26, 2016, 8:58 p.m., Anindya Sinha wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> -----------------------------------------------------------
> 
> (Updated Sept. 26, 2016, 8:58 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
>     https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Refactored `Resources::parse()` into 2 separate static functions:
> 1. Resources::fromJSONString() to parse JSON representation of
>    resources.
> 2. Resources::fromSimpleString() to parse text representation of
>    resources.
> 
> Since these 2 new functions return a `Try<vector<Resource>>`, the
> existing `Resources::parse()` implicitly converts that to a
> `Resources` object. This refactor is done to retrieve all resources
> (include empty resources) required for auto detection of root
> and MOUNT disks.
> 
> 
> Diffs
> -----
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 
> 
> Diff: https://reviews.apache.org/r/51999/diff/
> 
> 
> Testing
> -------
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>


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