----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44366/#review122638 -----------------------------------------------------------
Could you update the description to reflect the new state of the code? src/slave/containerizer/containerizer.cpp (line 96) <https://reviews.apache.org/r/44366/#comment184748> Can we also prevent fractional GPUs here? We'd also need to do fractional validation in the launch task path, inside `update` (as we discussed `prepare` is calling update so no need to stick it in both functions). src/slave/containerizer/containerizer.cpp (lines 97 - 102) <https://reviews.apache.org/r/44366/#comment184753> Since it's unlikely we'll have a default number of GPUs in the future, perhaps we just have a comment here that describes why we don't use a default? src/slave/containerizer/containerizer.cpp (line 114) <https://reviews.apache.org/r/44366/#comment184754> Needs to be in the ifdef, right? src/slave/containerizer/containerizer.cpp (lines 114 - 135) <https://reviews.apache.org/r/44366/#comment184756> Can you remove the periods at the end of the error messages? src/slave/containerizer/containerizer.cpp (lines 115 - 118) <https://reviews.apache.org/r/44366/#comment184758> Our pattern for error messages is to avoid the periods and multi-sentence messages. We'll try to write messages that can compose as follows: Failed to <action>: <reason> Where reason can also correspond to a composed message, so you can end up with things like: Failed to launch container: Failed to prepare container: Fractional 'gpus' not supported. So when we've needed multi-sentence messages, we've tended to use semi-colons. src/slave/containerizer/containerizer.cpp (line 118) <https://reviews.apache.org/r/44366/#comment184757> These should be plural, so 'gpus' src/slave/containerizer/containerizer.cpp (lines 121 - 130) <https://reviews.apache.org/r/44366/#comment184755> We can get rid of the parsing code here based on the suggestion in the previous review to add to common/parse.hpp. Note that we would usually use const& in the loop here. src/v1/resources.cpp (line 1238) <https://reviews.apache.org/r/44366/#comment184745> Don't feel obligated, but would be nice to do a blanket s/`.get().`/`->`/ cleanup in the resources / values files, separate from this patch. - Ben Mahler On March 8, 2016, 10:47 p.m., Kevin Klues wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44366/ > ----------------------------------------------------------- > > (Updated March 8, 2016, 10:47 p.m.) > > > Review request for mesos, Ben Mahler, Rob Todd, and Vikrama Ditya. > > > Bugs: MESOS-4865 > https://issues.apache.org/jira/browse/MESOS-4865 > > > Repository: mesos > > > Description > ------- > > Currently we can either infer the number of GPUs to offer as resources > from the --nvidia_gpus flag, or pass the amount explicity as a "gpus" > resource. In the future, we will generalize this via autodiscovery of > gpus and support for gpus types other than Nvidia. > > > Diffs > ----- > > include/mesos/resources.hpp 388c4f383b77be490cb2debb64a734c6d6e4a176 > include/mesos/v1/resources.hpp 64ad8bf08230aeaa173325364c91b765f091210e > src/common/resources.cpp cf0707209143084d86aff5e4427846f461479c54 > src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 > src/slave/constants.cpp 0f0d8e4b079d136d250f83cfc768de8c98b8bee2 > src/slave/containerizer/containerizer.cpp > f6fc7863d0c215611f170dc0c89aa229407b5137 > src/v1/resources.cpp 296c4c2392384a2fcc4f2c1843980ff97e71114d > > Diff: https://reviews.apache.org/r/44366/diff/ > > > Testing > ------- > > > Thanks, > > Kevin Klues > >
