----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66561/#review201378 -----------------------------------------------------------
src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp Lines 106 (patched) <https://reviews.apache.org/r/66561/#comment282620> Should we move this method to `src/hdfs/hdfs.cpp`? src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp Lines 133 (patched) <https://reviews.apache.org/r/66561/#comment282619> In which case can `host.empty()` be `true`? src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp Line 95 (original), 170 (patched) <https://reviews.apache.org/r/66561/#comment282601> Do we need to update the help message of `--docker_registry` to mention it can support `hdfs://`? Currently it only mentions `/tmp/docker/images` for local puller. src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp Lines 174 (patched) <https://reviews.apache.org/r/66561/#comment282621> Now in this patch, `parseHdfsUri()` can only return `uri::hdfs`. I am wondering if we can make it be able to return either `uri::hdfs` or `uri::file`, in this way, we do not need to pass `flags.docker_registry` into the constructor of `LocalPullerProcess` in line 185. src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp Lines 232-235 (patched) <https://reviews.apache.org/r/66561/#comment282637> Should we change `Option<string>()` and `Option<int>()` to `None()`? Or we could change it to something like: ``` URI uri = hdfsUri.get(); uri.set_path(paths::getImageArchiveTarPath(hdfsUri->path(), image)); ``` src/slave/containerizer/mesos/provisioner/docker/puller.cpp Lines 39-40 (original), 39-41 (patched) <https://reviews.apache.org/r/66561/#comment282594> [Image::Docker::name](https://github.com/apache/mesos/blob/1.5.0/include/mesos/v1/mesos.proto#L2696:L2700) can already represent docker registry, did you mean changing it in future? src/slave/containerizer/mesos/provisioner/docker/puller.cpp Lines 42-45 (patched) <https://reviews.apache.org/r/66561/#comment282638> We already support multiple (two) pullers, but just cannot be enabled simultaneously, so do you mean in future we may want to support these two pullers simultaneously? And can you elaborate a bit on why this would allow users to fetch image tarballs from 'http', 'https' etc. src/slave/containerizer/mesos/provisioner/docker/store.cpp Lines 148 (patched) <https://reviews.apache.org/r/66561/#comment282595> Just curious, I see this flag was introduced in Mesos long time ago, but I do not see this flag set in any Mesos code before, so this is the first time that we use it in Mesos? - Qian Zhang On April 17, 2018, 1:32 p.m., Gilbert Song wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66561/ > ----------------------------------------------------------- > > (Updated April 17, 2018, 1:32 p.m.) > > > Review request for mesos, Jie Yu and Qian Zhang. > > > Bugs: MESOS-8794 > https://issues.apache.org/jira/browse/MESOS-8794 > > > Repository: mesos > > > Description > ------- > > Supported hdfs fetching in local puller. > > > Diffs > ----- > > src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp > 4d2e4973a0d6c99dd3447a158003b4b09e2ba477 > src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp > 5ce49ac396b03e8b6d87601ecaa0691d88de21e3 > src/slave/containerizer/mesos/provisioner/docker/puller.cpp > d7d8987d493a37d20f32ddd254dc0c3b15159951 > src/slave/containerizer/mesos/provisioner/docker/store.cpp > 8b3f07f5027cb90d4b4ed401960494709d3eda5f > > > Diff: https://reviews.apache.org/r/66561/diff/2/ > > > Testing > ------- > > make check > > > Thanks, > > Gilbert Song > >
