----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34140/#review90873 -----------------------------------------------------------
src/slave/containerizer/provisioners/appc/store.hpp (lines 22 - 33) <https://reviews.apache.org/r/34140/#comment144684> Reorder includes. <std lib> <mesos/...> <process/...> <stout/...> src/slave/containerizer/provisioners/appc/store.hpp (lines 68 - 69) <https://reviews.apache.org/r/34140/#comment144036> I am sure you'll remove these but just putting on reminder here. src/slave/containerizer/provisioners/appc/store.hpp (lines 70 - 71) <https://reviews.apache.org/r/34140/#comment144038> Two empty lines used here. Kill one. src/slave/containerizer/provisioners/appc/store.hpp (line 95) <https://reviews.apache.org/r/34140/#comment144114> Move to the source file? src/slave/containerizer/provisioners/appc/store.cpp (line 103) <https://reviews.apache.org/r/34140/#comment144491> Suggestion: Somewhere in Store class if we draw a directory layout everything would be a lot clearer. src/slave/containerizer/provisioners/appc/store.cpp (line 148) <https://reviews.apache.org/r/34140/#comment144505> The code above captures the stage via `[=]()` so it's inconsistent here. Should be fine to keep using `[=]()`. src/slave/containerizer/provisioners/appc/store.cpp (lines 154 - 170) <https://reviews.apache.org/r/34140/#comment144123> TODO: This looks a bit hacky but that's what it takes to use the fetcher. That's some refactoring for the future. src/slave/containerizer/provisioners/appc/store.cpp (lines 188 - 189) <https://reviews.apache.org/r/34140/#comment144124> 4 space indentation. src/slave/containerizer/provisioners/appc/store.cpp (line 189) <https://reviews.apache.org/r/34140/#comment144515> Make "image.ci" a const variable. src/slave/containerizer/provisioners/appc/store.cpp (line 243) <https://reviews.apache.org/r/34140/#comment144683> 4 space indentation. src/slave/containerizer/provisioners/appc/store.cpp (line 247) <https://reviews.apache.org/r/34140/#comment144665> Indentation off by one space. src/slave/containerizer/provisioners/appc/store.cpp (line 268) <https://reviews.apache.org/r/34140/#comment144035> Just would like to mention the possibility of using `pigz` which speeds things up quite a bit but of course we need to check its availability. src/slave/containerizer/provisioners/appc/store.cpp (line 274) <https://reviews.apache.org/r/34140/#comment144136> Why different here? The other two formats have output being 'image'. FWIW I think image.aci.out looks clearer that it's a temp. src/slave/containerizer/provisioners/appc/store.cpp (line 276) <https://reviews.apache.org/r/34140/#comment144140> The availability of this binary is rearer than the previous two. And "xz not found" is treated the same as "file not compressed". Maybe at least some logging to distinguish the two cases. I understand that eventually the failure will occur in the untar step but it would be nice to see who the culprit is. (Maybe don't redirect stderr to /dev/null)? src/slave/containerizer/provisioners/appc/store.cpp (line 349) <https://reviews.apache.org/r/34140/#comment144192> Shift indentation left by one space. src/slave/containerizer/provisioners/appc/store.cpp (line 392) <https://reviews.apache.org/r/34140/#comment144196> "staging/XXXXXX to storage/hash"? src/slave/containerizer/provisioners/appc/store.cpp (line 397) <https://reviews.apache.org/r/34140/#comment144197> src/slave/containerizer/provisioners/appc/store.cpp (line 408) <https://reviews.apache.org/r/34140/#comment144198> What does this mean? TODO: rm store? We do need to remove the corrupted store to avoid accumulating too much space and at least avoid confusing. So how about: verify the image before renaming and do not move it into storage if it fails. The failed image is not immediately removed so the operator can log into the box to inspect it in the staging directoy but we know: 1) Images in the staging directory are going to be cleaned up by slave restart. 2) Images in the storage directory are all valid. src/slave/containerizer/provisioners/appc/store.cpp (line 412) <https://reviews.apache.org/r/34140/#comment144678> AppcImageManifest belongs to a later review /r/34142/. Maybe the manifest protobuf and its validation can be pulled out into a separate review that bears no depednency on store, discovery, etc and can be placed lower in the review depedency chain/tree? src/slave/containerizer/provisioners/appc/store.cpp (line 417) <https://reviews.apache.org/r/34140/#comment144199> Why have "sha512-" as id prefix but not on the path? It would be nice to have them be consistent. If id with sha512- prefix is clearer about the semantics of the id, perhaps using the full id (i.e. with the prefix) in the path also helps readability of the path. Can we can have a common helper method to convert hash to id, instead of currently there are two places that prepend the "sha512-" prefix. src/slave/containerizer/provisioners/appc/store.cpp (line 466) <https://reviews.apache.org/r/34140/#comment144485> No need for const. - 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/34140/ > ----------------------------------------------------------- > > (Updated July 7, 2015, 12:43 p.m.) > > > Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. > > > Repository: mesos > > > Description > ------- > > Images are fetched into the store (after discovery). Stored images are > currently kept indefinitely. > > > Diffs > ----- > > src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 > src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION > src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION > src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b > src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 > > Diff: https://reviews.apache.org/r/34140/diff/ > > > Testing > ------- > > > Thanks, > > Ian Downes > >
