-----------------------------------------------------------
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
>
>
|