> On June 24, 2015, 3:13 a.m., Bernd Mathiske wrote:
> > src/tests/docker_tests.cpp, line 374
> > <https://reviews.apache.org/r/35799/diff/1/?file=990751#file990751line374>
> >
> >     Use Flags::docker_sandbox_directory (or a constant that the flag also 
> > uses for its default value) instead. Not only will this fix the naked 
> > constant problem, it will also explain what this parameter does and make 
> > readers understand the test code much more quickly.
> >     
> >     I know this is copy-paste-induced, but the original is really, really 
> > bad. Please either fix now, or leave a TODO and a tech debt ticket.
> 
> Timothy Chen wrote:
>     We actually put anything here, so I just put the default settings. I can 
> just use /tmp/sandbox if that makes it more obvious?

What you have put there now ("directory.get()") does not provide the user with 
an idea what is going on without looking up the definition of the parameters of 
Docker::run() (nor would "/tmp/sandbox"). This may seem like a moot point, 
since it is indeed arbitrary what to put there. But conveying that information 
to the reader is actually the real point in question. So this fix is worse than 
the original IMHO. 

The previous string constant actually gave more of a hint and now I understand 
now why you named it "/mnt/mesos/sandbox". This kinda stands for 
"/arbitrary/mount/point/in/docker/for/the/Mesos/sandbox". Sorry for opening the 
issue in the first place! Please revert to "/mnt/mesos/sandbox". (Maybe add a 
helpful source code comment? :-)


- Bernd


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


On June 26, 2015, 4:26 p.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35799/
> -----------------------------------------------------------
> 
> (Updated June 26, 2015, 4:26 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-2374
>     https://issues.apache.org/jira/browse/MESOS-2374
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Support mounting relative paths with docker.
> 
> 
> Diffs
> -----
> 
>   src/docker/docker.cpp 62aac2773d8b74e19619cf593064165892b03ba6 
>   src/tests/docker_tests.cpp acf1e3c8f455e8b40e4b3cb8748fcfeacbab142c 
> 
> Diff: https://reviews.apache.org/r/35799/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Reply via email to