This is an automatically generated e-mail. To reply, visit:

Could you update the description to reflect the new state of the code?

src/slave/containerizer/containerizer.cpp (line 96)

    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)

    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)

    Needs to be in the ifdef, right?

src/slave/containerizer/containerizer.cpp (lines 114 - 135)

    Can you remove the periods at the end of the error messages?

src/slave/containerizer/containerizer.cpp (lines 115 - 118)

    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 

src/slave/containerizer/containerizer.cpp (line 118)

    These should be plural, so 'gpus'

src/slave/containerizer/containerizer.cpp (lines 121 - 130)

    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)

    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

Reply via email to