mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anindya Sinha <anindya_si...@apple.com>
Subject Re: Review Request 51879: Autodetect value of resource when not specified in static resources.
Date Thu, 01 Jun 2017 19:16:34 GMT


> On June 1, 2017, 12:42 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/containerizer.cpp
> > Lines 113 (patched)
> > <https://reviews.apache.org/r/51879/diff/13/?file=1731951#file1731951line175>
> >
> >     Sorry I know you had https://reviews.apache.org/r/52002 which was originally
intended for all disk types and later evolved into a single `bool isRootDisk()` but given
this is the only use of the helper can we just simply do the following here?
> >     
> >     ```
> >     bool isRootDisk = !resource.has_disk();
> >     ```
> >     
> >     Now I think a proper follow up would be to actually to give the root disk a
type which is set by default.
> >     
> >     e.g.,
> >     
> >     ```
> >         message Source {
> >           enum Type {
> >             UNKNOWN = 0;
> >             PATH = 1;
> >             MOUNT = 2;
> >             ROOT = 3;
> >           }
> >           ...
> >           optional Source source = 3 [default = ROOT];
> >         }
> >     ```
> >     
> >     Which would have made things much simpler in many places.

Dropped the RR https://reviews.apache.org/r/52002, and moved the other change in that RR to
https://reviews.apache.org/r/59724/.


> On June 1, 2017, 12:42 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/gpu/allocator.cpp
> > Lines 182-190 (original), 192-196 (patched)
> > <https://reviews.apache.org/r/51879/diff/13/?file=1731952#file1731952line192>
> >
> >     So if we are not auot-detecting for "gpus:" (which I think is fine for now as
an incremental step), are the changes in "src/slave/containerizer/mesos/isolators/gpu/allocator.cpp"
just a follow-up for https://reviews.apache.org/r/55761/?
> >     
> >     Then can we separate it out to simplify this patch?

Moved this part to a separate RR (https://reviews.apache.org/r/59723/) in this chain.


> On June 1, 2017, 12:42 a.m., Jiang Yan Xu wrote:
> > src/tests/persistent_volume_endpoints_tests.cpp
> > Lines 2180-2195 (patched)
> > <https://reviews.apache.org/r/51879/diff/13/?file=1731953#file1731953line2196>
> >
> >     Are these just reordering? Why do it?
> >     
> >     Same for other changes in this file?

In `Try<Resources> Containerizer::resources(const Flags& flags)`, we use a `const
hashset<string> predefined = {"cpus", "mem", "disk", "ports", "gpus"}` (since we call
a `contains()`). As a result, the order in which `vector<Resource> result` and hence
the `resources` returned from this function varies in order from before.

The test uses `JSON::Value` which should be enhanced to do a comparison using `Resources`
which we can do later. For now, I changed the order of the `JSON::Value` in the test to satisfy
the condition.


> On June 1, 2017, 12:42 a.m., Jiang Yan Xu wrote:
> > src/tests/reservation_endpoints_tests.cpp
> > Lines 1784-1791 (original)
> > <https://reviews.apache.org/r/51879/diff/13/?file=1731954#file1731954line1784>
> >
> >     Ditto. Why reordering?

Same reason as in previous comment.


- Anindya


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


On May 24, 2017, 7: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, 7: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