mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Timothy Chen" <tnac...@apache.org>
Subject Re: Review Request 37773: Docker: Adding registry client.
Date Fri, 28 Aug 2015 18:46:52 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37773/#review96905
-----------------------------------------------------------



src/slave/containerizer/provisioners/docker/registry_client.hpp (line 46)
<https://reviews.apache.org/r/37773/#comment152585>

    Why not just use option.getOrElse()?



src/slave/containerizer/provisioners/docker/registry_client.hpp (line 77)
<https://reviews.apache.org/r/37773/#comment152586>

    userId



src/slave/containerizer/provisioners/docker/registry_client.hpp (line 116)
<https://reviews.apache.org/r/37773/#comment152587>

    We usually spell timeout as one word everywhere else, so all lower case.



src/slave/containerizer/provisioners/docker/registry_client.hpp (line 150)
<https://reviews.apache.org/r/37773/#comment152588>

    Do we need to define static here if it's not being used anywhere else but the cpp files?



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 41)
<https://reviews.apache.org/r/37773/#comment152589>

    Kill extra line



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 87)
<https://reviews.apache.org/r/37773/#comment152593>

    What's the reasoning behind prefixing underscores for these? You've already added trailing
underscores for your class variables right?



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 91)
<https://reviews.apache.org/r/37773/#comment152590>

    getOrElse



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 114)
<https://reviews.apache.org/r/37773/#comment152592>

    I'm wondering why you need to use promise here.
    
    If you put all this logic in getManifest then essentially you just need to return the
dispatch right? And in getManifest you just chian your return with onFailed and onAny too.
    
    Also even if you don't move it in getManifest why can't you just return result.onFailed(...).onAny(...)?



src/slave/containerizer/provisioners/docker/registry_client.cpp (line 153)
<https://reviews.apache.org/r/37773/#comment152594>

    What residue?


- Timothy Chen


On Aug. 28, 2015, 6:38 p.m., Jojy Varghese wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37773/
> -----------------------------------------------------------
> 
> (Updated Aug. 28, 2015, 6:38 p.m.)
> 
> 
> Review request for mesos, Lily Chen and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added implementation for docker registry's Get Manifest and Get Blob APIs.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/provisioners/docker/registry_client.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/registry_client.cpp PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37773/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>


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