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




include/mesos/resources.hpp (lines 138 - 148)
<https://reviews.apache.org/r/51879/#comment227891>

    We don't need this method. See my response in the discussion for the 
previous revision.
    
    My main issue with this abstraction is that it tries to be generic but it 
can't and it has to give up for custom resources. If this is specifically for a 
few predefined kinds of resources then a few constants would be more intuitive:
    
    ```
    constexpr Value::Type CPUS_VALUE_TYPE = Value::SCALAR;
    ```
    
    But probably even that would be unnecessary for now.



include/mesos/resources.hpp (lines 150 - 156)
<https://reviews.apache.org/r/51879/#comment227890>

    We don't need the `containsValue` helper. See comments at the callsite. The 
main reason I am against it is that we are doing some special processing for 
auto-detection (on the agent, the scope is the key) in this patch which I don't 
feel we should generalize and encourage wider usage for the `Resources` class.



src/common/resources.cpp (line 510)
<https://reviews.apache.org/r/51879/#comment227462>

    If we don't support auto-detection for missing values in a simple string, 
we don't need to change this method?



src/common/resources.cpp (line 510)
<https://reviews.apache.org/r/51879/#comment227892>

    All of the changes here are equivalent to 
    
    ```
    Resource resource;
    resource.set_name(name);
    resource.set_role(role);
    ```
    
    at the callsite. We should probably do that instead.



src/common/resources.cpp (lines 636 - 638)
<https://reviews.apache.org/r/51879/#comment227919>

    If we want `Try<vector<Resource>> Resources::fromSimpleString(...)` to 
always return Resource objects that have types, I'd say that we do not provide 
support for auto-detection in the form of "cpus:" or "cpus" for simple strings. 
    
    Then we don't need to modify this method.



src/common/resources.cpp (lines 1922 - 1926)
<https://reviews.apache.org/r/51879/#comment227471>

    Enclose with `{}`.



src/common/resources.cpp (line 1927)
<https://reviews.apache.org/r/51879/#comment227468>

    No empty lines here.



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

    This comment seems redundant. This group of methods all do "auto-detect the 
size".



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

    Use `Option<string> path;` followed by `CHECK_SOME(path)` after all 
branches that set it?



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

    Use `switch (source.type())`?



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

    return an Error in this case. The input could be missing the mount source.



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

    return an Error in this case.



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

    This is a CHECK (we don't have a 3rd type yet so this "shouldn't happen"). 
If with a switch, this is simply
    
    ```
    switch (source.type()) {
      ...
      default: {
        UNREACHABLE();
      }
    }
    ```
    
    ?



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

    Add a comment about what the return value means?



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

    Nit: s/autodetect/detect/ 
    
    Also a simple comment that this method actually detect and fill in resource 
values only when they are missing?



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

    s/resources/parsed/



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

    s/all/resources/ (give s/resources/parsed/ above)?
    
    Mainly to avoid the confusion that this is for "all resources" when it's 
only "all resources of this name".



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

    We can just do 
    
    ```
    Resource resources;
    resources.set_name(name);
    resources.set_role(flags.default_role);
    ```



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

    We don't need this `containsValue` helper here because you still need to 
process each predefined resource separately: if a general purpose helper 
doesn't hide the specificity from the caller then we probably don't need it.



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

    Here we already know that we want "cpus", and we know it's a scalar/double, 
we can just check this way:
    
    ```
    if (name == "cpus" && !resource.has_scalar()) {
      _resource = Resources::parse(
          name,
          stringify(systemCpusAmount(),
          _resource.role()).get();
    }
    ```
    
    therefore we don't need the `containsVlaue` abstraction.



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

    Return an error if we cannot get the disk amount.



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

    This is not a "will never happen" case, return an error?



src/slave/containerizer/containerizer.cpp (lines 210 - 223)
<https://reviews.apache.org/r/51879/#comment227917>

    Can we consolidate the handling of each kind of resource into one place? 
    
    i.e.,
    
    ```
    Resource _resource = resource;
    
    // ... cpus, mem, etc.
    else if (name == "ports" && !_resource->has_ranges()) {
      _resource = Resources::parse(
          name,
          stringify(DEFAULT_PORTS),
          _resource->role()).get();
    }
    ```



src/slave/containerizer/containerizer.cpp (lines 225 - 234)
<https://reviews.apache.org/r/51879/#comment227916>

    We should just do this once, at the very end, before 
`Containerizer::resources()` returns and before we convert `vector<Resource>` 
into a `Resources`.
    
    Even when we do it there,
    
    ```
    Resources::isEmpty(_resource) 
    ```
    
    is not necessary as it's handled by the `+=` operator.
    
    We should return `Try<vector<Resource>>` in this method.



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

    Can we kill `parse()` and go straight from `Try<Resources> 
Containerizer::resources(const Flags& flags)` to individual `autodetect` 
methods?
    
    The semantics provided by `parse()` is not clear to me, it sounds like it 
only "parses the flags" but it does "almost" everything but still leaves out 
GPUs. The caller needs to do additional result checking and GPUs handling...



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

    s/resources/parsed/?



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

    Where are the custom resources added to the result?



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

    "except gpus" is not true? Yes it's delegates to another class but the 
logic is essentially the same there as well.



src/slave/containerizer/containerizer.cpp (lines 267 - 269)
<https://reviews.apache.org/r/51879/#comment227967>

    The way it's implemented, it's impossible for this to be error.



src/slave/containerizer/containerizer.cpp (lines 275 - 277)
<https://reviews.apache.org/r/51879/#comment227968>

    The way it's implemented, it's impossible for this to be error.



src/slave/containerizer/containerizer.cpp (lines 291 - 293)
<https://reviews.apache.org/r/51879/#comment227970>

    The way it's implemented, it's impossible for this to be error.



src/slave/containerizer/containerizer.cpp (lines 297 - 312)
<https://reviews.apache.org/r/51879/#comment227850>

    This additional validation of gpus is not necessary, we can handle 
everything gpus related in `Try<Resources> Containerizer::resources(const 
Flags& flags)`.



src/slave/containerizer/containerizer.cpp (lines 340 - 343)
<https://reviews.apache.org/r/51879/#comment227972>

    Fix indentation:
    
    ```
      resources = gpus.get() + resources.filter(
          [](const Resource& resource) {
            return resource.name() != "gpus";
          });
    ```



src/slave/containerizer/mesos/isolators/gpu/allocator.hpp (lines 57 - 59)
<https://reviews.apache.org/r/51879/#comment227973>

    We can keep the original interface.



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

    For now let's copy the code here so we don't have to modify the GPUs 
allocator interface.
    
    ```
      const string text = flags.resources.getOrElse("");
    
      // Try to parse as a JSON Array. Otherwise, parse as a text string.
      Try<JSON::Array> json = JSON::parse<JSON::Array>(text);
    
      Try<vector<Resource>> resources = json.isSome() ?
        Resources::fromJSON(json.get(), flags.default_role) :
        Resources::fromSimpleString(text, flags.default_role);
    
      if (resources.isError()) {
        return Error(resources.error());
      }
    ```
    
    We can do the `vector<Resource> resource::parse(...)` refactor later so 
this becomes a one liner and doesn't feel like much duplication.



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

    We can use the same "empty vector" condition to get rid of the unreliable 
`strings::contians()` check.


- Jiang Yan Xu


On Oct. 25, 2016, 11:19 a.m., Anindya Sinha wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51879/
> -----------------------------------------------------------
> 
> (Updated Oct. 25, 2016, 11:19 a.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.
> 
> With this change, JSON or textual 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 in JSON or textual formats
> 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.hpp 
> b2eabfebef99ccebef427d144bb816adc0175ecf 
>   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
> 
>

Reply via email to