mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bernd Mathiske" <be...@mesosphere.io>
Subject Re: Review Request 35799: Support mounting relative paths with docker.
Date Mon, 29 Jun 2015 12:36:21 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message