mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jie Yu <yujie....@gmail.com>
Subject Re: Review Request 56174: Added skipping already stored layers to local Docker puller.
Date Sun, 05 Feb 2017 01:28:09 GMT

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


Fix it, then Ship it!




I'll commit the code first without the test. Please follow up with the test using my suggestion!
Thanks!


src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp (lines 287 - 288)
<https://reviews.apache.org/r/56174/#comment235908>

    I typically wrap comments in 70 char width.



src/tests/containerizer/provisioner_docker_tests.cpp (lines 262 - 270)
<https://reviews.apache.org/r/56174/#comment235909>

    Instead of testing local puller directly, I'd suggest we test using 'Store'. This test
assumes too many impl details which will be very fragile as code evolves. We try to avoid
that if possible.
    
    My suggestion is that we pull the image once, making sure store has it. And then, delete
one layer, and then pull again. Make sure the second pull is successful.


- Jie Yu


On Feb. 3, 2017, 6:07 p.m., Ilya Pronin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56174/
> -----------------------------------------------------------
> 
> (Updated Feb. 3, 2017, 6:07 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Timothy Chen.
> 
> 
> Bugs: MESOS-7045
>     https://issues.apache.org/jira/browse/MESOS-7045
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> If a layer is already in the store, there's no need to extract it. `RegistryPuller` already
does this and `Store` is ready for such puller behaviour.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp ee391af898886bff9e5b911697f725c5ea53ebd8

>   src/tests/containerizer/provisioner_docker_tests.cpp af9987f88205d00d091f35fa734d5667506aaffd

> 
> Diff: https://reviews.apache.org/r/56174/diff/
> 
> 
> Testing
> -------
> 
> Added a test verifying that local puller will skip layes that are already in the store.
Ran `make check`.
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>


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