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