> On Sept. 18, 2016, 6:22 p.m., haosdent huang wrote: > > src/slave/containerizer/containerizer.cpp, lines 198-259 > > <https://reviews.apache.org/r/51879/diff/2/?file=1501142#file1501142line198> > > > > I suggest that we move the logic which parse `disk(0)` to resources.cpp > > or resources_utils.cpp.
`Resources` will never contain a resource which has a size of 0, since it does not add empty resources. That is the reason we refactored the `Resources::parse()` to the new function which returns a `Try<vector<Resource>>` instead of `Try<Resources>`. Since this logic is very specific to how `--resources` are handled, I think it is fine staying within this module, and I do not think it needs to move to resources.cpp or resources_utils.cpp (since these files deals specific to `Resources`). > On Sept. 18, 2016, 6:22 p.m., haosdent huang wrote: > > src/slave/containerizer/containerizer.cpp, lines 228-229 > > <https://reviews.apache.org/r/51879/diff/2/?file=1501142#file1501142line228> > > > > `.disk().source().type()` should be enough? We need to check for `has_disk()` [done via `Resources::isRootDisk()`], `disk().has_source()` and then retrieve `disk().source().type()`. I think having a single function for this makes sense to me. > On Sept. 18, 2016, 6:22 p.m., haosdent huang wrote: > > src/slave/containerizer/containerizer.hpp, lines 57-60 > > <https://reviews.apache.org/r/51879/diff/2/?file=1501141#file1501141line57> > > > > I think we should avoid adding the `diskSize` in the definition header > > of `containerizer` because it looks irrelevant. Suppose we would like to > > modular the containerizer, it would make us in trouble as the TODO > > mentioned below. > > > > ``` > > // TODO(idownes): Consider making this non-static and moving to > > // containerizer implementations to enable a containerizer to best > > // determine the resources, particularly if containerizeration is > > // delegated. > > static Try<Resources> resources(const Flags& flags); > > ``` > > > > `fs.hpp`, `resources.cpp` or `resources_utils.cpp` would be a better > > place? Removed from the `Containerizer` class, and made it an internal helper function (`static` function). > On Sept. 18, 2016, 6:22 p.m., haosdent huang wrote: > > src/slave/containerizer/containerizer.cpp, line 169 > > <https://reviews.apache.org/r/51879/diff/2/?file=1501142#file1501142line169> > > > > Use 0 to represent that determine the disk size automatically a bit > > nonintuitive. > > Please update docs/attributes-resources.md if we have to use this way > > eventually. Docs updated in https://reviews.apache.org/r/52071/. - Anindya ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51879/#review149374 ----------------------------------------------------------- On Sept. 18, 2016, 4:55 p.m., Anindya Sinha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51879/ > ----------------------------------------------------------- > > (Updated Sept. 18, 2016, 4:55 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 disks with a positive size, we use that > for the disk resources on the agent. However, --resources can include > disks with size of 0, which indicates that mesos agent determine the > size of those disks from the host and uses that information instead. > Note that this is not allowed for PATH disks since PATH disks can be > carved into smaller chunks and we cannot assume that the whole > physical disk is available to the PATH disk. > > With this change, JSON or textual representation for disk resources > that specify scalar value of 0 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 > ----- > > src/slave/containerizer/containerizer.hpp > f13669d0dfc4ce3287cfe630cabd0470dc765b51 > src/slave/containerizer/containerizer.cpp > d46882baa904fd439bffb23c324828b777228f1c > > Diff: https://reviews.apache.org/r/51879/diff/ > > > Testing > ------- > > Tests passed. > > > Thanks, > > Anindya Sinha > >
