You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jiang Yan Xu <ya...@jxu.me> on 2016/11/30 08:51:56 UTC
Re: Review Request 51879: Autodetect value of resource when not
specified in static resources.
-----------------------------------------------------------
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
>
>
Re: Review Request 51879: Autodetect value of resource when not
specified in static resources.
Posted by Anindya Sinha <an...@apple.com>.
> On Nov. 30, 2016, 8:51 a.m., Jiang Yan Xu wrote:
> > src/common/resources.cpp, line 1927
> > <https://reviews.apache.org/r/51879/diff/10/?file=1545305#file1545305line1927>
> >
> > No empty lines here.
Ok. Although I do see patterns without { } and / or with empty lines.
> On Nov. 30, 2016, 8:51 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/containerizer.cpp, line 150
> > <https://reviews.apache.org/r/51879/diff/10/?file=1545306#file1545306line150>
> >
> > Use `Option<string> path;` followed by `CHECK_SOME(path)` after all branches that set it?
Since `path` is set in all branches, I do not think we need an `Option<string>`, and hence no need for `CHECK_SOME()`. Right?
> On Nov. 30, 2016, 8:51 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/containerizer.cpp, line 297
> > <https://reviews.apache.org/r/51879/diff/10/?file=1545306#file1545306line297>
> >
> > Where are the custom resources added to the result?
Added now.
> On Nov. 30, 2016, 8:51 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/containerizer.cpp, line 304
> > <https://reviews.apache.org/r/51879/diff/10/?file=1545306#file1545306line304>
> >
> > "except gpus" is not true? Yes it's delegates to another class but the logic is essentially the same there as well.
I will calirify the comment. I meant to say that we do not autodetect `gpus` resources.
- Anindya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51879/#review157057
-----------------------------------------------------------
On Dec. 10, 2016, 1:19 a.m., Anindya Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51879/
> -----------------------------------------------------------
>
> (Updated Dec. 10, 2016, 1: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. 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
>
>