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

src/slave/containerizer/mesos/containerizer.cpp (line 65)

    Probably left over from a previous review?

src/slave/containerizer/provisioners/appc.hpp (line 54)

    Who outside appc.cpp uses it?

src/slave/containerizer/provisioners/appc.hpp (line 56)

    I see that this is used by Store. Possible to move to the Store base class 
to break circular dependency?

src/slave/containerizer/provisioners/appc.hpp (lines 58 - 61)

    Looks like this is only used in discovery. Can it be moved to Discovery so 
we can break the
    Dicovery -> Appc -> Discovery review dependency?

src/slave/containerizer/provisioners/appc.hpp (lines 63 - 67)

    Move to cpp if this is only used by appc.cpp?

src/slave/containerizer/provisioners/appc.hpp (lines 64 - 67)


src/slave/containerizer/provisioners/appc.hpp (line 93)


src/slave/containerizer/provisioners/appc.hpp (line 101)

    Move to cpp?

src/slave/containerizer/provisioners/appc.hpp (line 125)

    According the spec the ordering is significant so we better document it 
    In fact the implementation does return the list of image in **topological** 
order (dependencies go before dependents is the list), which is great. It 
doesn't address duplicates though, which is something we can improve.

src/slave/containerizer/provisioners/appc.hpp (line 127)

    s/hash/id/ ?
    As I commented on another review, I think it's less error-prone if we 
consistently use `id` rather than `hash`.

src/slave/containerizer/provisioners/appc.hpp (line 132)

    s/hash/id/ ?

src/slave/containerizer/provisioners/appc.hpp (line 148)

    What are additional fileds you imagine may be added in the future?

src/slave/containerizer/provisioners/appc.cpp (line 56)

    Such as checking `if(acKind == "ImageManifest")`?

src/slave/containerizer/provisioners/appc.cpp (line 84)

    IIUC about the spec there is not a "canonical name" but this is a template 
for SimpleDiscovery. e.g. MetaDiscovery, if one were to implement it, doesn't 
follow this.
    Can we move it to SimpleDisovery and make LocalDiscovery (which may be more 
aptly named LocalSimpleDiscovery?) a subclass of SimpleDiscovery?

src/slave/containerizer/provisioners/appc.cpp (line 113)

    Given the TODO above I am not sure if we still should do this at all. Maybe 
it's sufficient to just fill in the missing "arch" label and trust the operator 
will do the right thing?

src/slave/containerizer/provisioners/appc.cpp (lines 287 - 288)

    Inline this? Moreover, with the path manipulation getting hairy in the code 
(different places remove and append "rootfs") I think we'd better do something 
akin to **slave/paths.hpp**.

src/slave/containerizer/provisioners/appc.cpp (lines 322 - 323)

    This is probably where the lambda becomes to long and a callback with a 
descriptive name (the old style) is better?

src/slave/containerizer/provisioners/appc.cpp (lines 342 - 343)

    We typically break after the operator I think :) (e.g. 

src/slave/containerizer/provisioners/appc.cpp (lines 349 - 351)

    << 2 spaces

src/slave/containerizer/provisioners/appc.cpp (lines 365 - 375)

    Indentation: it should be two spaces right of the dot on `.then()`.

src/slave/containerizer/provisioners/appc.cpp (line 409)


src/slave/containerizer/provisioners/appc.cpp (line 423)

    fetchAll() already recursively traverses the dependency graph right?

src/slave/containerizer/provisioners/appc.cpp (line 424)

    There is possibility of poentially downloading identical images if done in 
parallel right? Also we need to maintain the image provisioning order.

src/slave/containerizer/provisioners/appc.cpp (line 446)

    Be more explicit with `Option<string>::none()`?

- Jiang Yan Xu

On July 7, 2015, 12:43 p.m., Ian Downes wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34142/
> -----------------------------------------------------------
> (Updated July 7, 2015, 12:43 p.m.)
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> Repository: mesos
> Description
> -------
> Discovers image(s), fetches to the image store, then provisions using
> a backend.
> Diffs
> -----
>   include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70 
>   src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 8c102fb7d1f79ee768cb06de3a976ea12f958712 
>   src/slave/containerizer/provisioners/appc.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc.cpp PRE-CREATION 
>   src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b 
>   src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 
> Diff: https://reviews.apache.org/r/34142/diff/
> Testing
> -------
> Thanks,
> Ian Downes

Reply via email to