> On Sept. 28, 2016, 3:37 p.m., Jiang Yan Xu wrote: > > src/common/resources.cpp, lines 587-618 > > <https://reviews.apache.org/r/51879/diff/6/?file=1510293#file1510293line587> > > > > No need for the helpers. Just the following is sufficient. > > > > ``` > > Resource resource; > > resource.set_name(name); > > resource.set_role(role); > > > > // Return a Resource with missing value. > > if (_value.isNone()) { > > return resource; > > } > > > > Value _value = result.get(); > > > > if (_value.type() == Value::SCALAR) { > > resource.set_type(Value::SCALAR); > > resource.mutable_scalar()->CopyFrom(_value.scalar()); > > } else if (_value.type() == Value::RANGES) { > > resource.set_type(Value::RANGES); > > resource.mutable_ranges()->CopyFrom(_value.ranges()); > > } else if (_value.type() == Value::SET) { > > resource.set_type(Value::SET); > > resource.mutable_set()->CopyFrom(_value.set()); > > } else { > > return Error( > > "Bad type for resource " + name + " value " + value + > > " type " + Value::Type_Name(_value.type())); > > } > > ``` > > Guangya Liu wrote: > Seems we still need the `type` for resource when `_value.isNone()`, so > the logic could be: > > ``` > Resource resource; > resource.set_name(name); > resource.set_role(role); > > // Return a Resource with missing value. > if (_value.isNone()) { > Try<Value::Type> _type = Resources::valueType(name); > if (_type.isError()) { > return Error(_type.error()); > } > > resource.set_type(_type.get()); > return resource; > } > > Value _value = result.get(); > > if (_value.type() == Value::SCALAR) { > resource.set_type(Value::SCALAR); > resource.mutable_scalar()->CopyFrom(_value.scalar()); > } else if (_value.type() == Value::RANGES) { > resource.set_type(Value::RANGES); > resource.mutable_ranges()->CopyFrom(_value.ranges()); > } else if (_value.type() == Value::SET) { > resource.set_type(Value::SET); > resource.mutable_set()->CopyFrom(_value.set()); > } else { > return Error( > "Bad type for resource " + name + " value " + value + > " type " + Value::Type_Name(_value.type())); > } > ``` > > Jiang Yan Xu wrote: > I don't think so. The type is based on the value. No value, no type. > After we detect the value, we'll have the type, no? > > Anindya Sinha wrote: > Firstly, I think type is based on resource name (for non-custom > resources), and value is based on resource name and type. > > Secondly, the function: > `Try<Resource> Resources::parse(const string& name, const string& value, > const string& role)` > > is always expected to return a valid `Resource` object (if there is no > error). By not setting the `type`, we make the `Resource` object invalid > since `type` is a `required` parameter. But not setting value does not make > `Resource` invalid since `scalar`, `ranges` and `set` are `optional` fields. > > Finally, we need JSON input to indicate `type` since it is `required` and > not specifying `type` (for resource with value missing in JSON format) would > fail `protobuf::parse<RepeatedPtrField<Resource>>(resourcesJSON)` in > `Resources::fromJSON()` since the protobuf parser expects `type` to be there. > > Hence, I think we should expect all input to indicate type (explicitly in > JSON, and implicitly based on resource name for text input). So, I think we > should add the resource type in `Try<Resource> Resources::parse(const string& > name, const string& value, const string& role)`.
Didn't realize this when I was first commenting but it looks like that the value of calling `Try<Resource> Resources::parse(const string& name, const string& value, const string& role)` with an empty value is basically zero... After calling the method which has to be changed to handle the special case, you have to assert that the resulting Try is a SOME. You might as well spend 3 lines to just do: ``` Resource resources; resources.set_name(name); resources.set_role(role); ``` Can we just do that? --- To be clear I do agree that Mesos should provide the types for predefined resources but I think: Ideally: - The user shouldn't need to specify types in either JSON or simple string format. - If the user does specify the type they should be validated. (We don't do that today). - The protobuf type of the predefined resource should come out of its own C++ type. It sucks that: - The JSON input has to specify the resource type just because protobuf requires it. --- For now: The reality is that we don't really have to support simple string auto-detection in the form of "cpus:" or "cpus"; they don't have feature parity with the JSON definition to begin with. I now think that it simplifies things a bit if we don't support it. Then we don't need to make `Try<vector<Resource>> Resources::fromSimpleString(...)` support inputs like "cpus:" or "cpus" without the `type`. Longer term: We can change the `Resources::type` from required to optional in the way similar to MESOS-4997. In fact, we don't even need to change the public APIs, just the internal APIs. > On Sept. 28, 2016, 3:37 p.m., Jiang Yan Xu wrote: > > src/slave/containerizer/mesos/isolators/gpu/allocator.cpp, lines 141-161 > > <https://reviews.apache.org/r/51879/diff/6/?file=1510296#file1510296line141> > > > > With the flags, the method here already has all information it needs. > > Let's delay the changes to gpus as this review is already large and more > > work is needed to make this better. > > Anindya Sinha wrote: > If `flags.resources` contains non-gpus resources with no value, then this > would fail here (owing to invalid resources) since the auto-detection logic > is contained here but local to `containerizer.cpp`. Hence, I exposed the > optional `Resources` arg for this function. If present, we just use that here > since the auto-detection logic has already been done for the call-site in > that case. If this arg is not provided or is `None()`, we continue with the > original flow. > > Note that I have not added auto-detection of gpus in this chain. Ok. However let's still keep the `Try<Resources> NvidiaGpuAllocator::resources(...)` API because the method is also called from `Try<Containerizer*> Containerizer::create(...)` and it's awkward to pre-parse the resources there. We can copy over the code that parses `--resources` flag into a vector. - Jiang Yan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51879/#review150615 ----------------------------------------------------------- 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 > >
