> On Sept. 23, 2015, 12:50 a.m., Jiang Yan Xu wrote: > > src/slave/containerizer/provisioner/docker/puller.hpp, line 60 > > <https://reviews.apache.org/r/38137/diff/14/?file=1081601#file1081601line60> > > > > If we use LinkedHashMap we don't need to create another struct right? > > Timothy Chen wrote: > True, but I actually like returning a list due to the fact we already > return a list when we collect all the futures, and thought converting it into > a LinkedHashMap in the end from a list was a bit unnecessary. What you think?
To me it's a bit contrived to explain this concept. Why is it LayerPath and not LayerInfo? Why is `id` a separate part of a "Path"? Why is the field inside be named `directory` instead of `path`? It's probably closer to a list of tuples of <ID, path>. I personally would spend a few more lines to do the conversion (which is an implementation detail) than exposing a concept that bears explanation to the caller. That said, it's not a big deal. Feel free to drop this. - Jiang Yan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38137/#review100165 ----------------------------------------------------------- On Sept. 24, 2015, 1:02 p.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38137/ > ----------------------------------------------------------- > > (Updated Sept. 24, 2015, 1:02 p.m.) > > > Review request for mesos, Jie Yu, Jojy Varghese, Till Toenshoff, and Jiang > Yan Xu. > > > Repository: mesos > > > Description > ------- > > Joining all the commits around provisioner and local store into one review so > it's easier to review, as patches > are changing code on top of each other. > > All the commits are going to committed together. > > > Diffs > ----- > > src/Makefile.am 776483b > src/slave/containerizer/provisioner/docker/local_puller.hpp PRE-CREATION > src/slave/containerizer/provisioner/docker/local_puller.cpp PRE-CREATION > src/slave/containerizer/provisioner/docker/message.hpp PRE-CREATION > src/slave/containerizer/provisioner/docker/message.proto PRE-CREATION > src/slave/containerizer/provisioner/docker/metadata_manager.hpp > PRE-CREATION > src/slave/containerizer/provisioner/docker/metadata_manager.cpp > PRE-CREATION > src/slave/containerizer/provisioner/docker/paths.hpp PRE-CREATION > src/slave/containerizer/provisioner/docker/paths.cpp PRE-CREATION > src/slave/containerizer/provisioner/docker/puller.hpp PRE-CREATION > src/slave/containerizer/provisioner/docker/puller.cpp PRE-CREATION > src/slave/containerizer/provisioner/docker/store.hpp PRE-CREATION > src/slave/containerizer/provisioner/docker/store.cpp PRE-CREATION > src/slave/containerizer/provisioner/store.cpp 35d1199 > src/slave/flags.hpp 3f6601a > src/slave/flags.cpp 8792162 > src/tests/containerizer/provisioner_docker_tests.cpp 1b0c304 > > Diff: https://reviews.apache.org/r/38137/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Timothy Chen > >
