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