mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Qian Zhang <zhq527...@gmail.com>
Subject Re: Review Request 66561: Supported hdfs fetching in local puller.
Date Wed, 18 Apr 2018 09:48:53 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message