> On March 9, 2016, 12:03 a.m., Ben Mahler wrote:
> > src/slave/flags.hpp, line 92
> > <https://reviews.apache.org/r/44365/diff/1/?file=1280277#file1280277line92>
> >
> >     Can you put this in an ifdef?

Done. I also moved it out of the `#ifdef __linux__ `directive.  Although we 
only support gpus on linux at the moment, there's nothing preventing us from 
supporting them on other platforms in the future. This should be treated as a 
generic flag in that respect (i.e. nvidia-smi is also a command that can be run 
on windows).


> On March 9, 2016, 12:03 a.m., Ben Mahler wrote:
> > src/slave/flags.cpp, lines 597-607
> > <https://reviews.apache.org/r/44365/diff/1/?file=1280278#file1280278line597>
> >
> >     -s/nvidia_gpus/nvidia_gpu_devices/
> >     -capitalize NVML
> >     -s/as a resource/as resources/
> >     -maybe put the nvidia-smi comment in a parenthetical?
> >     -The third sentence seems to be already expressed in the first? Perhaps 
> > clarify that the isolator will be used only if the `--isolation` flag 
> > contains the right value.
> >     -We can probably omit or re-pharse the last sentence since this flag is 
> > omitted from the program if the configure flag is not present.

Done. I also updated the string in `docs/configuration.md`.  In the `.md` file, 
I left the (slightly reworded) last line, since there is no way of `#ifdef`ing 
this flag out in the documentation.


> On March 9, 2016, 12:03 a.m., Ben Mahler wrote:
> > src/slave/flags.cpp, lines 606-607
> > <https://reviews.apache.org/r/44365/diff/1/?file=1280278#file1280278line606>
> >
> >     I was going to suggest we add a validator here, but even better would 
> > be to add a parse specialization to src/common/parse.hpp and just avoid the 
> > need for the extra validation altogether.

Because it is so generic, I decided to add this parser to stout instead of 
mesos.  It should appear as a a dependency of this patch once I push it to 
reviewboard. As part of this, I now set `vector<const unsigned int>` as the 
return type on the `nvidia_gpu_devices` flag.


- Kevin


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


On March 14, 2016, 7:38 a.m., Kevin Klues wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44365/
> -----------------------------------------------------------
> 
> (Updated March 14, 2016, 7:38 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Rob Todd, and Vikrama Ditya.
> 
> 
> Bugs: MESOS-4864
>     https://issues.apache.org/jira/browse/MESOS-4864
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added flag to specify available Nvidia GPUs on an agent's command line.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md 739d4ff9aeeb1ba70ce11033168d63d37b6ef56b 
>   src/slave/flags.hpp feb095da4521f678c96f4cc53bdfda262d350388 
>   src/slave/flags.cpp 5bacf109bcd9c9eae1b7ec3c32095c72899ebdbd 
> 
> Diff: https://reviews.apache.org/r/44365/diff/
> 
> 
> Testing
> -------
> 
> ./bin/mesos-slave.sh --master=127.0.0.1:5050 --nvidia_gpu_devices=1,2,string
> Failed to load flag 'nvidia_gpu_devices': Failed to load value '1,2,string': 
> Expecting all elements to be unsigned integers
> 
> ./bin/mesos-slave.sh --master=127.0.0.1:5050 --nvidia_gpu_devices=1,2.0,3   
> Failed to load flag 'nvidia_gpu_devices': Failed to load value '1,2,3.0': 
> Expecting all elements to be unsigned integers
> 
> ./bin/mesos-slave.sh --master=127.0.0.1:5050 --nvidia_gpu_devices=1,2,3
> SUCCESS
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Reply via email to