mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Timothy Chen" <tnac...@apache.org>
Subject Re: Review Request 29334: Add option to launch docker containers with helper containers.
Date Tue, 07 Apr 2015 00:48:49 GMT


> On Jan. 18, 2015, 12:18 p.m., Bernd Mathiske wrote:
> > src/slave/containerizer/docker.hpp, line 219
> > <https://reviews.apache.org/r/29334/diff/5/?file=824296#file824296line219>
> >
> >     IMHO readability is subverted by prolonging the underscore scheme when there
is no strict series of continuations. This code would be so much more easy to read if there
were descriptive method names. Absent these, please add comments that summarize the purpose
and general approach of these methods in places such as this one.

The comments is in the cpp code itself, I'm not inclined to add comments in the header since
this is not really used outside.


> On Jan. 18, 2015, 12:18 p.m., Bernd Mathiske wrote:
> > src/slave/containerizer/docker.hpp, line 196
> > <https://reviews.apache.org/r/29334/diff/5/?file=824296#file824296line196>
> >
> >     At this compleity level, the comments here have begun to look like an anti-pattern
that might create unwanted precedent for others to mimic. IMHO this was already the case before
this patch, but the current additions exacerbate it.
> >     
> >     The naming launchInContainer helps, but it is still in line with the expected
style? 
> >     
> >     It seems to me that the expected style breaks down here. Ideas?

I think this is going to stay until we have a better naming scheme.


- Timothy


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


On April 7, 2015, 12:46 a.m., Timothy Chen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29334/
> -----------------------------------------------------------
> 
> (Updated April 7, 2015, 12:46 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Bernd Mathiske.
> 
> 
> Bugs: MESOS-2183
>     https://issues.apache.org/jira/browse/MESOS-2183
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add option to launch docker containers with helper containers.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/docker.hpp b7bf54a 
>   src/slave/containerizer/docker.cpp 5f4b4ce 
> 
> Diff: https://reviews.apache.org/r/29334/diff/
> 
> 
> Testing
> -------
> 
> make, tests are fixed in next commit
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>


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