mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiang Yan Xu <...@jxu.me>
Subject Re: Review Request 51999: Refactor parsing of resources.
Date Tue, 27 Sep 2016 17:28:01 GMT

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




include/mesos/resources.hpp (lines 56 - 64)
<https://reviews.apache.org/r/51999/#comment218530>

    Per my reply to a previous comment, can we kill this?



include/mesos/resources.hpp (line 165)
<https://reviews.apache.org/r/51999/#comment218482>

    The JSON string is still text and the argument is called `text`.
    
    Can we keep the original summary?



include/mesos/resources.hpp 
<https://reviews.apache.org/r/51999/#comment218601>

    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.
       */
    ```



include/mesos/resources.hpp (line 187)
<https://reviews.apache.org/r/51999/#comment218531>

    Not yours, but s/cpus"/"cpus"/



include/mesos/resources.hpp (line 195)
<https://reviews.apache.org/r/51999/#comment218483>

    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 = "*")
    ```



include/mesos/resources.hpp (lines 522 - 524)
<https://reviews.apache.org/r/51999/#comment218532>

    No need for this.



src/common/resources.cpp (lines 434 - 450)
<https://reviews.apache.org/r/51999/#comment218533>

    Kill the method and use the contents to implement `fromJSONString`



src/common/resources.cpp (lines 456 - 457)
<https://reviews.apache.org/r/51999/#comment218534>

    Not yours, but 2 space indentation, could you fix it now that we are refactoring?



src/common/resources.cpp (line 461)
<https://reviews.apache.org/r/51999/#comment218535>

    Not yours but wrap operators at the end of the line for consistency.



src/common/resources.cpp (lines 473 - 477)
<https://reviews.apache.org/r/51999/#comment218536>

    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.



src/common/resources.cpp (lines 580 - 610)
<https://reviews.apache.org/r/51999/#comment218591>

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



src/common/resources.cpp (lines 618 - 625)
<https://reviews.apache.org/r/51999/#comment218593>

    This method itself pratically does nothing in this form. We can simply move the contents
of `convertJSON` over.



src/common/resources.cpp (lines 669 - 673)
<https://reviews.apache.org/r/51999/#comment218596>

    Same as `fromJSONString()`, kill this.



src/tests/resources_tests.cpp (lines 630 - 657)
<https://reviews.apache.org/r/51999/#comment218600>

    With out refactor, the result of `Resources::parse()` doesn't change right?


- Jiang Yan Xu


On Sept. 26, 2016, 1: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, 1: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