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 Wed, 31 May 2017 21:33:34 GMT


> On Dec. 15, 2016, 11:58 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/containerizer.cpp
> > Lines 165-169 (original), 288-305 (patched)
> > <https://reviews.apache.org/r/51879/diff/11/?file=1581274#file1581274line327>
> >
> >     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.
> 
> Anindya Sinha wrote:
>     I removed the following block from `Try<vector<Resource>> detect()` since
if the `name` does not match the resources that cannot be auto-detected, it would be a no-op,
ie. returns the resources with no modifications.
>     
>     ```
>     if (name != "cpus" && name != "mem" && name != "disk" &&
name != "ports") {
>       return Error(
>         "Auto-detection of resource type '" + name + "' not supported");
>     }
>     ```
>     
>     I think I would still want to keep the `detect()` function separate. Seems cleaner
to me, otherwise we would have to have bunch of `if` conditions within this `foreach` for
each of the predefined resource types. How does this look instead?
>     
>     ```
>       const hashset<string> predefined = ({"cpus", "mem", "disk", "ports", "gpus"});
>     
>       foreach (const string& name, predefined) {
>     #ifdef __linux__
>         // GPU resource is 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
>     
>         if (name != "gpus") {
>           Try<vector<Resource>> _resources = detect(name, flags, parsed.get());
>           if (_resources.isError()) {
>             return Error(
>                 "Failed to obtain " + name + " resources: " + _resources.error());
>           }
>     
>           result.insert(
>               result.end(), _resources.get().begin(), _resources.get().end());
>         }
>       }
>     
>       // Custom resources.
>       foreach (const Resource& resource, parsed.get()) {
>         if (!predefined.contains(resource.name())) {
>           result.push_back(resource);
>         }
>       }
>     ```

This is a reiteration of the comment above and I am inclined to get this shipped first and
then work on a refactor.

But for posterity, to summarize in a short example, I don't think this is a good pattern.

```
void genericHelper(condition)
{
  // common code.
  
  switch (something) {
    case A:
      customHelperA();
    case B:
      customHelperB();
  }
}

void highLevelMethod()
{
  switch (condition) {
    case A:
      genericHelper(c1);
    case B:
      genericHelper(c2);
  }
}
```

In this case the generic helper is not really generic. In the high level method if we already
have to enumerate specific cases, we should be able to directly call the custom helpers.

To address of the concern of "code duplication" we should be able to just pull out the common
code into the high-level method. I think we can do something similar to: https://github.com/apache/mesos/blob/0bc502292d51c7d78c01de5764c5280b3fc0e3df/src/slave/containerizer/mesos/containerizer.cpp#L289

But again, it can be a follow-up.


- Jiang Yan


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


On May 24, 2017, 12:56 p.m., Anindya Sinha wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51879/
> -----------------------------------------------------------
> 
> (Updated May 24, 2017, 12:56 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 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 74583e3c784f22b45cce81f07c5a69b339e1684d 
>   include/mesos/v1/resources.hpp 132ef3eb03d335774ba13ecb39045128d0476954 
>   src/common/resources.cpp 1d07f1a049ba3bb3f5d78f05031f33ba97e07e8c 
>   src/slave/containerizer/containerizer.cpp 15ae0b33a5ea8f73a9a84081d9c310c1d59c3141

>   src/slave/containerizer/mesos/isolators/gpu/allocator.cpp c288ad634b856702483b9751f41445308babd0c9

>   src/tests/persistent_volume_endpoints_tests.cpp 8c54372b7f6d94f0335561086f2a8cb90373e285

>   src/tests/reservation_endpoints_tests.cpp 8c195eb7d788fbca8722d66d81c77d399702160a

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


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