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 51879: Autodetect value of resource when not specified in static resources.
Date Fri, 16 Dec 2016 07:58:58 GMT

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



LGTM! Just some minor comments.


src/common/resources.cpp (lines 513 - 514)
<https://reviews.apache.org/r/51879/#comment230287>

    If we do change this, I think using "with" and ": " looks better? 
    
    ```
    "Bad type for resource '" + name + "' with value '" + value +
    "': " + Value::Type_Name(_value.type()));
    ```
    
    - ":" because type is already mentioned in the beginning of the sentense.
    - Not quoting type because it's not mixed in the sentense anymore and it doesn't contain
spaces.
    
    Also the other error in the same method still uses a different style, change both for
consistency?



src/common/resources.cpp (lines 604 - 606)
<https://reviews.apache.org/r/51879/#comment230289>

    The `Resource` class doesn't know anything about auto-detection so the comment here is
not too relevant?
    
    We already have comments elsewhere to explain this.



src/slave/containerizer/containerizer.cpp (line 61)
<https://reviews.apache.org/r/51879/#comment230293>

    We acutally aren't leaving any headroom for cpus.



src/slave/containerizer/containerizer.cpp (line 104)
<https://reviews.apache.org/r/51879/#comment230296>

    Remove the `.`.



src/slave/containerizer/containerizer.cpp (line 173)
<https://reviews.apache.org/r/51879/#comment230412>

    `type` here is ambiguous. In this context there's already a `type` that means SCALAR/RANGES/SET.
    
    Quoted `Resources` means the `Resources` object which we don't return here.
    
    How about 
    
    ```
    Return a vector of `Resource` objects for predefined resources, viz. "cpus", "mem", "disk"
and "ports". The amount of resources are detected if unspecified.
    ```



src/slave/containerizer/containerizer.cpp (line 174)
<https://reviews.apache.org/r/51879/#comment230297>

    s/amount/the amount/



src/slave/containerizer/containerizer.cpp (line 183)
<https://reviews.apache.org/r/51879/#comment230413>

    s/type //



src/slave/containerizer/containerizer.cpp (line 188)
<https://reviews.apache.org/r/51879/#comment230414>

    s/resource/resources/



src/slave/containerizer/containerizer.cpp (lines 206 - 256)
<https://reviews.apache.org/r/51879/#comment230415>

    I suggested this previously:
    
    ```
    vector<Resource> result;
    
    foreach (Resource& resource, resources) {
      Resource _resource = resource;
      
      if (name == "cpus" && !resource.has_scalar()) {
        _resource = Resources::parse(
            name,
            stringify(systemCpusAmount()),
            _resource.role()).get();
        
        // The following is fine too.
        // _resource.set_type(Value::SCALAR);
        // _resource.mutable_scalar()->set_value(systemCpusAmount());
      } else if (name == "mem" && !resource.has_scalar()) {
        _resource = Resources::parse(
            name,
            stringify(systemMemAmount().megabytes()),
            _resource.role()).get();
      } else if (name == "disk" && !resource.has_scalar()) {
        Try<Bytes> amount = diskAmount(resource, flags);
    
        if (amount.isError()) {
          return Error("Failed to retrieve disk size: " + _size.error());
        }
        
        _resource.set_type(Value::SCALAR);
        _resource.mutable_scalar()->set_value(amount.megabytes());
      } else if (name == "ports" && !resource.has_ranges())) {
        _resource = Resources::parse(
            name,
            stringify(DEFAULT_PORTS),
            _resource.role()).get();
      }
      
      result.push_back(_resource);
    }
    ```
    
    The advantage is that this 
    - avoids scattering special handling of each kind of resource in multiple places.
    - avoids explaining the "defaultSize" (which would be clearer if named `detected`)
    - avoids `internal::values::parse()`.



src/slave/containerizer/containerizer.cpp (lines 271 - 273)
<https://reviews.apache.org/r/51879/#comment230397>

    My bad in suggesting this indentation style, looks like the commonly used style is:
    
    ```
    Try<vector<Resource>> parsed = json.isSome()
      ? Resources::fromJSON(json.get(), flags.default_role)
      : Resources::fromSimpleString(text, flags.default_role);
    ```



src/slave/containerizer/containerizer.cpp (line 281)
<https://reviews.apache.org/r/51879/#comment230408>

    Nit: s/resources/resource/ since singular is used below?



src/slave/containerizer/containerizer.cpp (lines 282 - 284)
<https://reviews.apache.org/r/51879/#comment230402>

    We do auto-detect 'missing' cpus in the simple string representation as well so this is
inaccurate.
    
    How about
    
    ```
    We auto-detect "cpus" when:
    (1) no cpus is specified; or
    (2) cpus is specified with no value.
    
    Note (2) is currently only possible when JSON input is used with `--resources` because
the parsing logic requires the resource's `type` to be specified (as it's a required protobuf
field).
    ```



src/slave/containerizer/containerizer.cpp (line 287)
<https://reviews.apache.org/r/51879/#comment230403>

    "We" (Mesos) do auto-detect "gpus": https://github.com/apache/mesos/blob/ead829bfff3256793185dbeb53906016874c48d7/src/slave/containerizer/mesos/isolators/gpu/allocator.cpp#L241
    
    How about:
    
    ```
    The same logic applies for the other known resource types. Note that "gpus" auto-detection
is handled in `NvidiaGpuAllocator::resources()`.
    ```



src/slave/containerizer/containerizer.cpp (lines 288 - 305)
<https://reviews.apache.org/r/51879/#comment230434>

    Here what I found to be a bit awkward:
    
    We already know here that we want to process "cpus", "mem", "disk" and "ports". We don't
do it directly but we call a helper method which has to check that the input are precisely
what this method provides.
    
    ```
    if (name != "cpus" && name != "mem" && name != "disk" && name
!= "ports") {
      return Error(
        "Auto-detection of resource type '" + name + "' not supported");
    }
    ```
    
    and from this method we have the check the output.
    
    This makes the helper not a natrual abstraction but rather a tightly coupled blocked to
code to avoid duplicating logic for all predefined resources. If that's the objective, then
we can just do:
    
    ```
    const hashset<string> predefined = ({"cpus", "mem", "disk", "ports", "gpus"};
    
    foreach (const string& name, predefined) {
    #ifdef __linux__
      // GPUS are handled separately.
      if (name == "gpus") {
        Try<Resources> gpus = NvidiaGpuAllocator::resources(flags);
        if (gpus.isError()) {
          return Error("Failed to obtain GPU resources: " + gpus.error());
        }
    
        foreach(const Resource& gpu, gpus.get()) {
          result.push_back(gpu);
        }
      }
    #endif
      
      // Content of `detect()` except we can just add stuff to `result` without returning.
    }
    
    // Custom resources.
    foreach(const Resource& resource, parsed.get()) {
      if (!predefined.contains(resource.name())) {
        result.push_back(resource);
      }
    }
    ```
    
    The result should be less amount of code without any increase in code duplication.



src/slave/containerizer/containerizer.cpp (line 314)
<https://reviews.apache.org/r/51879/#comment230410>

    Space after `foreach`.



src/slave/containerizer/containerizer.cpp (lines 321 - 323)
<https://reviews.apache.org/r/51879/#comment230299>

    The following more elegant?
    
    ```
    const hashset<string> predefined = ({"cpus", "mem", "disk", "ports", "gpus"};
    
    ...
    
    foreach(const Resource& resource, parsed.get()) {
      if (!predefined.contains(resource.name())) {
        result.push_back(resource);
      }
    }
    ```



src/slave/containerizer/containerizer.cpp (line 328)
<https://reviews.apache.org/r/51879/#comment230416>

    `+=` validates the individual 'Resource' objects too. Here I guess we want to highlight
that:
    
    ```
    // Explicit validation because we want to propagte the error.
    ```



src/slave/containerizer/containerizer.cpp (line 330)
<https://reviews.apache.org/r/51879/#comment230411>

    Space after `foreach`.



src/slave/containerizer/mesos/isolators/gpu/allocator.cpp (lines 158 - 160)
<https://reviews.apache.org/r/51879/#comment230422>

    Ditto about style.



src/slave/containerizer/mesos/isolators/gpu/allocator.cpp (line 167)
<https://reviews.apache.org/r/51879/#comment230421>

    Space after `foreach`.



src/slave/containerizer/mesos/isolators/gpu/allocator.cpp (lines 192 - 196)
<https://reviews.apache.org/r/51879/#comment230429>

    It's the same logic to support GPUs that are missing values right? Can we add it (fine
to be in a separate review)? Otherwise we have to separately document that we don't support
GPUs and the inconsistency looks bad.


- Jiang Yan Xu


On Dec. 9, 2016, 5:19 p.m., Anindya Sinha wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51879/
> -----------------------------------------------------------
> 
> (Updated Dec. 9, 2016, 5:19 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
>     https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> When static resources indicate resources with a positive size, we use
> that for the resources on the agent. However, --resources can include
> resources with no size, which indicates that mesos agent determine the
> size of those resources from the agent and uses that information. Note
> that auto-detection of resources is allowed for all known resource
> types except "gpus" (i.e. "cpus", "mem", "disk" and "ports") when
> represented in JSON format only. Auto-detection is not done when the
> resources are represented in text format.
> 
> With this change, JSON representation for disk resources that do not
> specify any value would not result in an error, but those resources
> will not be accounted for until a valid size is determined for such
> resources. A scalar value of -1 still results in an invalid resource.
> 
> 
> Diffs
> -----
> 
>   include/mesos/resources.hpp f569c931ff7db8d51dfd7c96f4f2addab05df85d 
>   include/mesos/v1/resources.hpp f60ab794a0c7c24885c49cc47b798c363e3279e7 
>   src/common/resources.cpp 4bb9beffcb3509f4226b4985e05eccec01412d0f 
>   src/slave/containerizer/containerizer.cpp d46882baa904fd439bffb23c324828b777228f1c

>   src/slave/containerizer/mesos/isolators/gpu/allocator.cpp 2e722691475c84afae14009014ea70cc0fdd0e65

>   src/v1/resources.cpp 46cc00f2f453f5eb4ddc4b0b9b89be2bd89f05d9 
> 
> Diff: https://reviews.apache.org/r/51879/diff/
> 
> 
> Testing
> -------
> 
> Tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>


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