You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jojy Varghese <jo...@mesosphere.io> on 2015/11/01 16:38:20 UTC
Re: Review Request 38580: Added docker registry RemotePuller
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38580/
-----------------------------------------------------------
(Updated Nov. 1, 2015, 3:38 p.m.)
Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu.
Changes
-------
rebased.
Repository: mesos
Description
-------
Integrated remote puller with store. Tests will follow.
Diffs (updated)
-----
src/CMakeLists.txt d107e329cc6887cd9d4ce3706dfc6ce6080d0289
src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db
src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 87d80026939a326bd1169f46906e36d6ef19f546
src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp f314f20d989ed0125e15d2a14d0f8e56c56976d5
src/slave/containerizer/mesos/provisioner/docker/puller.hpp 8010b8aa75a2fc2e4b537f915f9dca028e407b63
src/slave/containerizer/mesos/provisioner/docker/puller.cpp f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2
src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp ad57d8592c5f9df5115b575855ed2e99a0597359
src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c
src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp PRE-CREATION
src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp PRE-CREATION
src/slave/containerizer/mesos/provisioner/docker/store.cpp bb02d650e16d45fcf337a7954f7a26143fb2c69f
src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73
src/slave/flags.cpp ed9b0b8313f5a5e53f3715af5300d9fcaa936df8
src/tests/containerizer/provisioner_docker_tests.cpp 99c2af7d06dd7908e8d77e1bfb3e20622fb3eb4c
Diff: https://reviews.apache.org/r/38580/diff/
Testing
-------
make check.
Thanks,
Jojy Varghese
Re: Review Request 38580: Added docker registry RemotePuller
Posted by Timothy Chen <tn...@apache.org>.
> On Nov. 4, 2015, 9:50 p.m., Timothy Chen wrote:
> > src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp, line 236
> > <https://reviews.apache.org/r/38580/diff/19/?file=1115760#file1115760line236>
> >
> > Let's use consistent failure messaging here.
> > If you like to include the layer id please include in both cases, and also specify the layer as well for all cases.
>
> Jojy Varghese wrote:
> Not sure i understand.
You have promise->fail here that includes "Failed to download layer", but in the else branch you don't.
Also for discarded you don't also print out why it failed, but then in the else branch you print out the failure message.
What I hope is that we have a consistent message in the beginning for all cases:
"Failed to download layer '" + layer.layerId + "': " isDiscarded() ? "future discarded" : failure()
Does this makes sense?
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38580/#review105134
-----------------------------------------------------------
On Nov. 4, 2015, 6:09 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38580/
> -----------------------------------------------------------
>
> (Updated Nov. 4, 2015, 6:09 p.m.)
>
>
> Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Integrated remote puller with store. Tests will follow.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt d107e329cc6887cd9d4ce3706dfc6ce6080d0289
> src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db
> src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 87d80026939a326bd1169f46906e36d6ef19f546
> src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp f314f20d989ed0125e15d2a14d0f8e56c56976d5
> src/slave/containerizer/mesos/provisioner/docker/puller.hpp 8010b8aa75a2fc2e4b537f915f9dca028e407b63
> src/slave/containerizer/mesos/provisioner/docker/puller.cpp f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2
> src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp ad57d8592c5f9df5115b575855ed2e99a0597359
> src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c
> src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp PRE-CREATION
> src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp PRE-CREATION
> src/slave/containerizer/mesos/provisioner/docker/store.cpp bb02d650e16d45fcf337a7954f7a26143fb2c69f
> src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73
> src/slave/flags.cpp ed9b0b8313f5a5e53f3715af5300d9fcaa936df8
> src/tests/containerizer/provisioner_docker_tests.cpp 8d90894410cd834edf49a2814d1b616718798fe8
>
> Diff: https://reviews.apache.org/r/38580/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 38580: Added docker registry RemotePuller
Posted by Timothy Chen <tn...@apache.org>.
> On Nov. 4, 2015, 9:50 p.m., Timothy Chen wrote:
> > src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp, line 129
> > <https://reviews.apache.org/r/38580/diff/19/?file=1115760#file1115760line129>
> >
> > Move the timeout after into the RemotePullerProcess.
>
> Jojy Varghese wrote:
> Since RemotePullerProcess does many sub-steps, how do I set a timer for all of the steps aggregated?
I think if we are going to have a generic puller timeout, let's put the after in the store then, and discard the future outside.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38580/#review105134
-----------------------------------------------------------
On Nov. 4, 2015, 6:09 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38580/
> -----------------------------------------------------------
>
> (Updated Nov. 4, 2015, 6:09 p.m.)
>
>
> Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Integrated remote puller with store. Tests will follow.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt d107e329cc6887cd9d4ce3706dfc6ce6080d0289
> src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db
> src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 87d80026939a326bd1169f46906e36d6ef19f546
> src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp f314f20d989ed0125e15d2a14d0f8e56c56976d5
> src/slave/containerizer/mesos/provisioner/docker/puller.hpp 8010b8aa75a2fc2e4b537f915f9dca028e407b63
> src/slave/containerizer/mesos/provisioner/docker/puller.cpp f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2
> src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp ad57d8592c5f9df5115b575855ed2e99a0597359
> src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c
> src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp PRE-CREATION
> src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp PRE-CREATION
> src/slave/containerizer/mesos/provisioner/docker/store.cpp bb02d650e16d45fcf337a7954f7a26143fb2c69f
> src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73
> src/slave/flags.cpp ed9b0b8313f5a5e53f3715af5300d9fcaa936df8
> src/tests/containerizer/provisioner_docker_tests.cpp 8d90894410cd834edf49a2814d1b616718798fe8
>
> Diff: https://reviews.apache.org/r/38580/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 38580: Added docker registry RemotePuller
Posted by Timothy Chen <tn...@apache.org>.
> On Nov. 4, 2015, 9:50 p.m., Timothy Chen wrote:
> > src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp, line 295
> > <https://reviews.apache.org/r/38580/diff/19/?file=1115760#file1115760line295>
> >
> > Are you trying to check for custom registry?
> >
> > The Name struct has an optional registry field that you should check instead:
> >
> > if (name.has_registry()) {
> > return Error(.....);
> > }
>
> Jojy Varghese wrote:
> The issue is that we dont know what is being passed as Image::Name. Is it the local name or canonical name. If its local name, checking for "/" is sufficient. The idea is that we dont accept any paths in the name.
I'm not sure I really understand the difference between local and canonical, but whatever image name people have been using with docker client is what the name we're expecting the user to give us as well. I believe it's local name, and we don't need to really expect another format.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38580/#review105134
-----------------------------------------------------------
On Nov. 4, 2015, 6:09 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38580/
> -----------------------------------------------------------
>
> (Updated Nov. 4, 2015, 6:09 p.m.)
>
>
> Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Integrated remote puller with store. Tests will follow.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt d107e329cc6887cd9d4ce3706dfc6ce6080d0289
> src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db
> src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 87d80026939a326bd1169f46906e36d6ef19f546
> src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp f314f20d989ed0125e15d2a14d0f8e56c56976d5
> src/slave/containerizer/mesos/provisioner/docker/puller.hpp 8010b8aa75a2fc2e4b537f915f9dca028e407b63
> src/slave/containerizer/mesos/provisioner/docker/puller.cpp f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2
> src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp ad57d8592c5f9df5115b575855ed2e99a0597359
> src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c
> src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp PRE-CREATION
> src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp PRE-CREATION
> src/slave/containerizer/mesos/provisioner/docker/store.cpp bb02d650e16d45fcf337a7954f7a26143fb2c69f
> src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73
> src/slave/flags.cpp ed9b0b8313f5a5e53f3715af5300d9fcaa936df8
> src/tests/containerizer/provisioner_docker_tests.cpp 8d90894410cd834edf49a2814d1b616718798fe8
>
> Diff: https://reviews.apache.org/r/38580/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 38580: Added docker registry RemotePuller
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On Nov. 4, 2015, 9:50 p.m., Timothy Chen wrote:
> > src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp, line 51
> > <https://reviews.apache.org/r/38580/diff/19/?file=1115760#file1115760line51>
> >
> > Didn''t you pull these out?
>
> Jojy Varghese wrote:
> Not sure what you mean.
No its part of the RegistryPuller class.
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38580/#review105134
-----------------------------------------------------------
On Nov. 8, 2015, 4:58 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38580/
> -----------------------------------------------------------
>
> (Updated Nov. 8, 2015, 4:58 p.m.)
>
>
> Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Integrated remote puller with store. Tests will follow.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt cbc25e3ea33c47a787d34a7fa8499af3eb0b2c10
> src/Makefile.am c479acafc92b7e9d078729d443b0d2f00d40f353
> src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 87d80026939a326bd1169f46906e36d6ef19f546
> src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp f314f20d989ed0125e15d2a14d0f8e56c56976d5
> src/slave/containerizer/mesos/provisioner/docker/puller.hpp 8010b8aa75a2fc2e4b537f915f9dca028e407b63
> src/slave/containerizer/mesos/provisioner/docker/puller.cpp f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2
> src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp bc487d34e4087fa53452d63416d0f31e77060eb0
> src/slave/containerizer/mesos/provisioner/docker/registry_puller.hpp PRE-CREATION
> src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp PRE-CREATION
> src/slave/containerizer/mesos/provisioner/docker/store.cpp bb02d650e16d45fcf337a7954f7a26143fb2c69f
> src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73
> src/slave/flags.cpp 825b6127dfbc39cb3aea852875eaaab95bac0516
> src/tests/containerizer/provisioner_docker_tests.cpp fe6a90fe32364eec8ef923a000db19183603c338
>
> Diff: https://reviews.apache.org/r/38580/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 38580: Added docker registry RemotePuller
Posted by Timothy Chen <tn...@apache.org>.
> On Nov. 4, 2015, 9:50 p.m., Timothy Chen wrote:
> > src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp, line 373
> > <https://reviews.apache.org/r/38580/diff/19/?file=1115760#file1115760line373>
> >
> > Instead of doing this, how about just create a new Image::Name?
>
> Jojy Varghese wrote:
> The intent here is that we create the new image name exactly as the one passed one expect the repository name.
Which is I thought it's weird that we have to also set another name, I would thought having a helper function that just gives us the right remote Image::Name and we put all the details there is better.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38580/#review105134
-----------------------------------------------------------
On Nov. 4, 2015, 6:09 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38580/
> -----------------------------------------------------------
>
> (Updated Nov. 4, 2015, 6:09 p.m.)
>
>
> Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Integrated remote puller with store. Tests will follow.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt d107e329cc6887cd9d4ce3706dfc6ce6080d0289
> src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db
> src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 87d80026939a326bd1169f46906e36d6ef19f546
> src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp f314f20d989ed0125e15d2a14d0f8e56c56976d5
> src/slave/containerizer/mesos/provisioner/docker/puller.hpp 8010b8aa75a2fc2e4b537f915f9dca028e407b63
> src/slave/containerizer/mesos/provisioner/docker/puller.cpp f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2
> src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp ad57d8592c5f9df5115b575855ed2e99a0597359
> src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c
> src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp PRE-CREATION
> src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp PRE-CREATION
> src/slave/containerizer/mesos/provisioner/docker/store.cpp bb02d650e16d45fcf337a7954f7a26143fb2c69f
> src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73
> src/slave/flags.cpp ed9b0b8313f5a5e53f3715af5300d9fcaa936df8
> src/tests/containerizer/provisioner_docker_tests.cpp 8d90894410cd834edf49a2814d1b616718798fe8
>
> Diff: https://reviews.apache.org/r/38580/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 38580: Added docker registry RemotePuller
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On Nov. 4, 2015, 9:50 p.m., Timothy Chen wrote:
> > src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp, line 51
> > <https://reviews.apache.org/r/38580/diff/19/?file=1115760#file1115760line51>
> >
> > Didn''t you pull these out?
Not sure what you mean.
> On Nov. 4, 2015, 9:50 p.m., Timothy Chen wrote:
> > src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp, line 129
> > <https://reviews.apache.org/r/38580/diff/19/?file=1115760#file1115760line129>
> >
> > Move the timeout after into the RemotePullerProcess.
Since RemotePullerProcess does many sub-steps, how do I set a timer for all of the steps aggregated?
> On Nov. 4, 2015, 9:50 p.m., Timothy Chen wrote:
> > src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp, line 133
> > <https://reviews.apache.org/r/38580/diff/19/?file=1115760#file1115760line133>
> >
> > Let the caller LOG instead of us since we already forward all necessary information to the caller.
I wanted context of the failure logged out at the source of it to help debug issues.
> On Nov. 4, 2015, 9:50 p.m., Timothy Chen wrote:
> > src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp, line 236
> > <https://reviews.apache.org/r/38580/diff/19/?file=1115760#file1115760line236>
> >
> > Let's use consistent failure messaging here.
> > If you like to include the layer id please include in both cases, and also specify the layer as well for all cases.
Not sure i understand.
> On Nov. 4, 2015, 9:50 p.m., Timothy Chen wrote:
> > src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp, line 295
> > <https://reviews.apache.org/r/38580/diff/19/?file=1115760#file1115760line295>
> >
> > Are you trying to check for custom registry?
> >
> > The Name struct has an optional registry field that you should check instead:
> >
> > if (name.has_registry()) {
> > return Error(.....);
> > }
The issue is that we dont know what is being passed as Image::Name. Is it the local name or canonical name. If its local name, checking for "/" is sufficient. The idea is that we dont accept any paths in the name.
> On Nov. 4, 2015, 9:50 p.m., Timothy Chen wrote:
> > src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp, line 373
> > <https://reviews.apache.org/r/38580/diff/19/?file=1115760#file1115760line373>
> >
> > Instead of doing this, how about just create a new Image::Name?
The intent here is that we create the new image name exactly as the one passed one expect the repository name.
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38580/#review105134
-----------------------------------------------------------
On Nov. 4, 2015, 6:09 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38580/
> -----------------------------------------------------------
>
> (Updated Nov. 4, 2015, 6:09 p.m.)
>
>
> Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Integrated remote puller with store. Tests will follow.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt d107e329cc6887cd9d4ce3706dfc6ce6080d0289
> src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db
> src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 87d80026939a326bd1169f46906e36d6ef19f546
> src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp f314f20d989ed0125e15d2a14d0f8e56c56976d5
> src/slave/containerizer/mesos/provisioner/docker/puller.hpp 8010b8aa75a2fc2e4b537f915f9dca028e407b63
> src/slave/containerizer/mesos/provisioner/docker/puller.cpp f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2
> src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp ad57d8592c5f9df5115b575855ed2e99a0597359
> src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c
> src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp PRE-CREATION
> src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp PRE-CREATION
> src/slave/containerizer/mesos/provisioner/docker/store.cpp bb02d650e16d45fcf337a7954f7a26143fb2c69f
> src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73
> src/slave/flags.cpp ed9b0b8313f5a5e53f3715af5300d9fcaa936df8
> src/tests/containerizer/provisioner_docker_tests.cpp 8d90894410cd834edf49a2814d1b616718798fe8
>
> Diff: https://reviews.apache.org/r/38580/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 38580: Added docker registry RemotePuller
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38580/#review105134
-----------------------------------------------------------
src/Makefile.am (line 564)
<https://reviews.apache.org/r/38580/#comment163530>
We use tab indentation in the Makefilm.am
src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp (line 57)
<https://reviews.apache.org/r/38580/#comment163540>
I think this is redudant.
src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp (line 69)
<https://reviews.apache.org/r/38580/#comment163541>
We talked about removing these right? And it's not matching the actual params
src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp (line 43)
<https://reviews.apache.org/r/38580/#comment163542>
We don't use alias namespaces tpyically.
src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp (line 51)
<https://reviews.apache.org/r/38580/#comment163543>
Didn''t you pull these out?
src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp (line 129)
<https://reviews.apache.org/r/38580/#comment163544>
Move the timeout after into the RemotePullerProcess.
src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp (line 133)
<https://reviews.apache.org/r/38580/#comment163546>
Let the caller LOG instead of us since we already forward all necessary information to the caller.
src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp (line 213)
<https://reviews.apache.org/r/38580/#comment163548>
Remove extra space
src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp (line 216)
<https://reviews.apache.org/r/38580/#comment163547>
Fix indentation
src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp (line 236)
<https://reviews.apache.org/r/38580/#comment163553>
Let's use consistent failure messaging here.
If you like to include the layer id please include in both cases, and also specify the layer as well for all cases.
src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp (line 295)
<https://reviews.apache.org/r/38580/#comment163555>
Are you trying to check for custom registry?
The Name struct has an optional registry field that you should check instead:
if (name.has_registry()) {
return Error(.....);
}
src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp (line 303)
<https://reviews.apache.org/r/38580/#comment163556>
Can you rebase on all your refactoring patches?
src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp (line 373)
<https://reviews.apache.org/r/38580/#comment163558>
Instead of doing this, how about just create a new Image::Name?
src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp (line 380)
<https://reviews.apache.org/r/38580/#comment163559>
Remove extra space between ' and :
src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp (line 385)
<https://reviews.apache.org/r/38580/#comment163560>
ditto
src/slave/flags.cpp (line 118)
<https://reviews.apache.org/r/38580/#comment163537>
This is only used in the remote puller for now, so perhaps say "for pulling images from the Docker registry"
src/slave/flags.cpp (line 123)
<https://reviews.apache.org/r/38580/#comment163538>
How about "Default Docker registry server host"
src/slave/flags.cpp (line 128)
<https://reviews.apache.org/r/38580/#comment163539>
Default Docker registry server port
- Timothy Chen
On Nov. 4, 2015, 6:09 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38580/
> -----------------------------------------------------------
>
> (Updated Nov. 4, 2015, 6:09 p.m.)
>
>
> Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Integrated remote puller with store. Tests will follow.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt d107e329cc6887cd9d4ce3706dfc6ce6080d0289
> src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db
> src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 87d80026939a326bd1169f46906e36d6ef19f546
> src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp f314f20d989ed0125e15d2a14d0f8e56c56976d5
> src/slave/containerizer/mesos/provisioner/docker/puller.hpp 8010b8aa75a2fc2e4b537f915f9dca028e407b63
> src/slave/containerizer/mesos/provisioner/docker/puller.cpp f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2
> src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp ad57d8592c5f9df5115b575855ed2e99a0597359
> src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c
> src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp PRE-CREATION
> src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp PRE-CREATION
> src/slave/containerizer/mesos/provisioner/docker/store.cpp bb02d650e16d45fcf337a7954f7a26143fb2c69f
> src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73
> src/slave/flags.cpp ed9b0b8313f5a5e53f3715af5300d9fcaa936df8
> src/tests/containerizer/provisioner_docker_tests.cpp 8d90894410cd834edf49a2814d1b616718798fe8
>
> Diff: https://reviews.apache.org/r/38580/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 38580: Added docker registry RemotePuller
Posted by Timothy Chen <tn...@apache.org>.
> On Nov. 4, 2015, 10:05 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp, line 43
> > <https://reviews.apache.org/r/38580/diff/19/?file=1115760#file1115760line43>
> >
> > This was done in another patch by Ben M (currently in registry_client.cpp). So i thought the style was safe.
I think you're right, feel free to drop it.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38580/#review105151
-----------------------------------------------------------
On Nov. 4, 2015, 6:09 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38580/
> -----------------------------------------------------------
>
> (Updated Nov. 4, 2015, 6:09 p.m.)
>
>
> Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Integrated remote puller with store. Tests will follow.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt d107e329cc6887cd9d4ce3706dfc6ce6080d0289
> src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db
> src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 87d80026939a326bd1169f46906e36d6ef19f546
> src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp f314f20d989ed0125e15d2a14d0f8e56c56976d5
> src/slave/containerizer/mesos/provisioner/docker/puller.hpp 8010b8aa75a2fc2e4b537f915f9dca028e407b63
> src/slave/containerizer/mesos/provisioner/docker/puller.cpp f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2
> src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp ad57d8592c5f9df5115b575855ed2e99a0597359
> src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c
> src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp PRE-CREATION
> src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp PRE-CREATION
> src/slave/containerizer/mesos/provisioner/docker/store.cpp bb02d650e16d45fcf337a7954f7a26143fb2c69f
> src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73
> src/slave/flags.cpp ed9b0b8313f5a5e53f3715af5300d9fcaa936df8
> src/tests/containerizer/provisioner_docker_tests.cpp 8d90894410cd834edf49a2814d1b616718798fe8
>
> Diff: https://reviews.apache.org/r/38580/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 38580: Added docker registry RemotePuller
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38580/#review105151
-----------------------------------------------------------
src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp (line 43)
<https://reviews.apache.org/r/38580/#comment163564>
This was done in another patch by Ben M (currently in registry_client.cpp). So i thought the style was safe.
- Jojy Varghese
On Nov. 4, 2015, 6:09 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38580/
> -----------------------------------------------------------
>
> (Updated Nov. 4, 2015, 6:09 p.m.)
>
>
> Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Integrated remote puller with store. Tests will follow.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt d107e329cc6887cd9d4ce3706dfc6ce6080d0289
> src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db
> src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 87d80026939a326bd1169f46906e36d6ef19f546
> src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp f314f20d989ed0125e15d2a14d0f8e56c56976d5
> src/slave/containerizer/mesos/provisioner/docker/puller.hpp 8010b8aa75a2fc2e4b537f915f9dca028e407b63
> src/slave/containerizer/mesos/provisioner/docker/puller.cpp f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2
> src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp ad57d8592c5f9df5115b575855ed2e99a0597359
> src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c
> src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp PRE-CREATION
> src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp PRE-CREATION
> src/slave/containerizer/mesos/provisioner/docker/store.cpp bb02d650e16d45fcf337a7954f7a26143fb2c69f
> src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73
> src/slave/flags.cpp ed9b0b8313f5a5e53f3715af5300d9fcaa936df8
> src/tests/containerizer/provisioner_docker_tests.cpp 8d90894410cd834edf49a2814d1b616718798fe8
>
> Diff: https://reviews.apache.org/r/38580/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 38580: Added docker registry RemotePuller
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38580/#review105512
-----------------------------------------------------------
src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp (line 408)
<https://reviews.apache.org/r/38580/#comment164128>
I don't think we're adding any value here logging this. Same as onfailed as well, let's just leave it to the caller since we don't have much to say.
- Timothy Chen
On Nov. 6, 2015, 5:36 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38580/
> -----------------------------------------------------------
>
> (Updated Nov. 6, 2015, 5:36 p.m.)
>
>
> Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Integrated remote puller with store. Tests will follow.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt d107e329cc6887cd9d4ce3706dfc6ce6080d0289
> src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db
> src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 87d80026939a326bd1169f46906e36d6ef19f546
> src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp f314f20d989ed0125e15d2a14d0f8e56c56976d5
> src/slave/containerizer/mesos/provisioner/docker/puller.hpp 8010b8aa75a2fc2e4b537f915f9dca028e407b63
> src/slave/containerizer/mesos/provisioner/docker/puller.cpp f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2
> src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp ad57d8592c5f9df5115b575855ed2e99a0597359
> src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp PRE-CREATION
> src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp PRE-CREATION
> src/slave/containerizer/mesos/provisioner/docker/store.cpp bb02d650e16d45fcf337a7954f7a26143fb2c69f
> src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73
> src/slave/flags.cpp ed9b0b8313f5a5e53f3715af5300d9fcaa936df8
> src/tests/containerizer/provisioner_docker_tests.cpp 8d90894410cd834edf49a2814d1b616718798fe8
>
> Diff: https://reviews.apache.org/r/38580/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 38580: Added docker registry RemotePuller
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38580/
-----------------------------------------------------------
(Updated Nov. 8, 2015, 4:58 p.m.)
Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu.
Changes
-------
review
Repository: mesos
Description
-------
Integrated remote puller with store. Tests will follow.
Diffs (updated)
-----
src/CMakeLists.txt cbc25e3ea33c47a787d34a7fa8499af3eb0b2c10
src/Makefile.am c479acafc92b7e9d078729d443b0d2f00d40f353
src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 87d80026939a326bd1169f46906e36d6ef19f546
src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp f314f20d989ed0125e15d2a14d0f8e56c56976d5
src/slave/containerizer/mesos/provisioner/docker/puller.hpp 8010b8aa75a2fc2e4b537f915f9dca028e407b63
src/slave/containerizer/mesos/provisioner/docker/puller.cpp f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2
src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp bc487d34e4087fa53452d63416d0f31e77060eb0
src/slave/containerizer/mesos/provisioner/docker/registry_puller.hpp PRE-CREATION
src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp PRE-CREATION
src/slave/containerizer/mesos/provisioner/docker/store.cpp bb02d650e16d45fcf337a7954f7a26143fb2c69f
src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73
src/slave/flags.cpp 825b6127dfbc39cb3aea852875eaaab95bac0516
src/tests/containerizer/provisioner_docker_tests.cpp fe6a90fe32364eec8ef923a000db19183603c338
Diff: https://reviews.apache.org/r/38580/diff/
Testing
-------
make check.
Thanks,
Jojy Varghese
Re: Review Request 38580: Added docker registry RemotePuller
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On Nov. 7, 2015, 9:36 a.m., Timothy Chen wrote:
> > src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp, line 51
> > <https://reviews.apache.org/r/38580/diff/24/?file=1118727#file1118727line51>
> >
> > This is no longer true since we pulled these out of RemotePuller right? Same as all the other ones.
No its part of RegistryPuller
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38580/#review105579
-----------------------------------------------------------
On Nov. 8, 2015, 4:58 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38580/
> -----------------------------------------------------------
>
> (Updated Nov. 8, 2015, 4:58 p.m.)
>
>
> Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Integrated remote puller with store. Tests will follow.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt cbc25e3ea33c47a787d34a7fa8499af3eb0b2c10
> src/Makefile.am c479acafc92b7e9d078729d443b0d2f00d40f353
> src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 87d80026939a326bd1169f46906e36d6ef19f546
> src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp f314f20d989ed0125e15d2a14d0f8e56c56976d5
> src/slave/containerizer/mesos/provisioner/docker/puller.hpp 8010b8aa75a2fc2e4b537f915f9dca028e407b63
> src/slave/containerizer/mesos/provisioner/docker/puller.cpp f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2
> src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp bc487d34e4087fa53452d63416d0f31e77060eb0
> src/slave/containerizer/mesos/provisioner/docker/registry_puller.hpp PRE-CREATION
> src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp PRE-CREATION
> src/slave/containerizer/mesos/provisioner/docker/store.cpp bb02d650e16d45fcf337a7954f7a26143fb2c69f
> src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73
> src/slave/flags.cpp 825b6127dfbc39cb3aea852875eaaab95bac0516
> src/tests/containerizer/provisioner_docker_tests.cpp fe6a90fe32364eec8ef923a000db19183603c338
>
> Diff: https://reviews.apache.org/r/38580/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 38580: Added docker registry RemotePuller
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On Nov. 7, 2015, 9:36 a.m., Timothy Chen wrote:
> > src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp, line 246
> > <https://reviews.apache.org/r/38580/diff/24/?file=1118727#file1118727line246>
> >
> > Can there be layers that actually have zero content? Docker images AFAIK has the last layer left empty as a mutable layer, so I'm wondering if you could actually get empty layer from an image.
Not sure. I tried ubuntu and busybox images and couldnt get a "empty" layer.
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38580/#review105579
-----------------------------------------------------------
On Nov. 8, 2015, 4:58 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38580/
> -----------------------------------------------------------
>
> (Updated Nov. 8, 2015, 4:58 p.m.)
>
>
> Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Integrated remote puller with store. Tests will follow.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt cbc25e3ea33c47a787d34a7fa8499af3eb0b2c10
> src/Makefile.am c479acafc92b7e9d078729d443b0d2f00d40f353
> src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 87d80026939a326bd1169f46906e36d6ef19f546
> src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp f314f20d989ed0125e15d2a14d0f8e56c56976d5
> src/slave/containerizer/mesos/provisioner/docker/puller.hpp 8010b8aa75a2fc2e4b537f915f9dca028e407b63
> src/slave/containerizer/mesos/provisioner/docker/puller.cpp f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2
> src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp bc487d34e4087fa53452d63416d0f31e77060eb0
> src/slave/containerizer/mesos/provisioner/docker/registry_puller.hpp PRE-CREATION
> src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp PRE-CREATION
> src/slave/containerizer/mesos/provisioner/docker/store.cpp bb02d650e16d45fcf337a7954f7a26143fb2c69f
> src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73
> src/slave/flags.cpp 825b6127dfbc39cb3aea852875eaaab95bac0516
> src/tests/containerizer/provisioner_docker_tests.cpp fe6a90fe32364eec8ef923a000db19183603c338
>
> Diff: https://reviews.apache.org/r/38580/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 38580: Added docker registry RemotePuller
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38580/#review105579
-----------------------------------------------------------
src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp (line 45)
<https://reviews.apache.org/r/38580/#comment164189>
We pull multiple images with each puller, I would clarify that "Pulls docker images from docker registry"
src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp (line 47)
<https://reviews.apache.org/r/38580/#comment164190>
We had quite some feeedback about not really knowing what does Remote mean in RemotePuller. How about let's rename this to RegistryPuller instead?
I think this matches what it does more closely, since a Registry could technically even live in the same box.
src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp (line 51)
<https://reviews.apache.org/r/38580/#comment164191>
This is no longer true since we pulled these out of RemotePuller right? Same as all the other ones.
src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp (line 131)
<https://reviews.apache.org/r/38580/#comment164198>
Move this into RegistryPullerProcess, introduce a new method inside to call after with.
src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp (line 246)
<https://reviews.apache.org/r/38580/#comment164192>
Can there be layers that actually have zero content? Docker images AFAIK has the last layer left empty as a mutable layer, so I'm wondering if you could actually get empty layer from an image.
src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp (line 365)
<https://reviews.apache.org/r/38580/#comment164193>
I would delete onDiscard and onFailed here.
Not sure if moving this to the top makes any difference.
src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp (line 376)
<https://reviews.apache.org/r/38580/#comment164194>
you don't need process::defer here? You skipped the namespace here but used it in Line 226
src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp (line 381)
<https://reviews.apache.org/r/38580/#comment164195>
Fix the indentation as we discussed earlier,
foreach (const .......,
manifest.fsLayerInfos) {
}
src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp (line 391)
<https://reviews.apache.org/r/38580/#comment164196>
Move these before the return line.
src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp (line 394)
<https://reviews.apache.org/r/38580/#comment164197>
Can we move these callbacks into smaller named methods?
I think it makes the flow much clearer:
return registryClient->getManifest()
.then(.... return downloadLayers()
.then(.... return untarLayers())
- Timothy Chen
On Nov. 7, 2015, 12:11 a.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38580/
> -----------------------------------------------------------
>
> (Updated Nov. 7, 2015, 12:11 a.m.)
>
>
> Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Integrated remote puller with store. Tests will follow.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt cbc25e3ea33c47a787d34a7fa8499af3eb0b2c10
> src/Makefile.am 736f81252712bc4eee2c114535c325777a85a78f
> src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 87d80026939a326bd1169f46906e36d6ef19f546
> src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp f314f20d989ed0125e15d2a14d0f8e56c56976d5
> src/slave/containerizer/mesos/provisioner/docker/puller.hpp 8010b8aa75a2fc2e4b537f915f9dca028e407b63
> src/slave/containerizer/mesos/provisioner/docker/puller.cpp f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2
> src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp bc487d34e4087fa53452d63416d0f31e77060eb0
> src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp PRE-CREATION
> src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp PRE-CREATION
> src/slave/containerizer/mesos/provisioner/docker/store.cpp bb02d650e16d45fcf337a7954f7a26143fb2c69f
> src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73
> src/slave/flags.cpp ed9b0b8313f5a5e53f3715af5300d9fcaa936df8
> src/tests/containerizer/provisioner_docker_tests.cpp fe6a90fe32364eec8ef923a000db19183603c338
>
> Diff: https://reviews.apache.org/r/38580/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 38580: Added docker registry RemotePuller
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38580/
-----------------------------------------------------------
(Updated Nov. 7, 2015, 12:11 a.m.)
Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu.
Changes
-------
review
Repository: mesos
Description
-------
Integrated remote puller with store. Tests will follow.
Diffs (updated)
-----
src/CMakeLists.txt cbc25e3ea33c47a787d34a7fa8499af3eb0b2c10
src/Makefile.am 736f81252712bc4eee2c114535c325777a85a78f
src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 87d80026939a326bd1169f46906e36d6ef19f546
src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp f314f20d989ed0125e15d2a14d0f8e56c56976d5
src/slave/containerizer/mesos/provisioner/docker/puller.hpp 8010b8aa75a2fc2e4b537f915f9dca028e407b63
src/slave/containerizer/mesos/provisioner/docker/puller.cpp f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2
src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp bc487d34e4087fa53452d63416d0f31e77060eb0
src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp PRE-CREATION
src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp PRE-CREATION
src/slave/containerizer/mesos/provisioner/docker/store.cpp bb02d650e16d45fcf337a7954f7a26143fb2c69f
src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73
src/slave/flags.cpp ed9b0b8313f5a5e53f3715af5300d9fcaa936df8
src/tests/containerizer/provisioner_docker_tests.cpp fe6a90fe32364eec8ef923a000db19183603c338
Diff: https://reviews.apache.org/r/38580/diff/
Testing
-------
make check.
Thanks,
Jojy Varghese
Re: Review Request 38580: Added docker registry RemotePuller
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38580/
-----------------------------------------------------------
(Updated Nov. 6, 2015, 5:36 p.m.)
Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu.
Changes
-------
rebased
Repository: mesos
Description
-------
Integrated remote puller with store. Tests will follow.
Diffs (updated)
-----
src/CMakeLists.txt d107e329cc6887cd9d4ce3706dfc6ce6080d0289
src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db
src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 87d80026939a326bd1169f46906e36d6ef19f546
src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp f314f20d989ed0125e15d2a14d0f8e56c56976d5
src/slave/containerizer/mesos/provisioner/docker/puller.hpp 8010b8aa75a2fc2e4b537f915f9dca028e407b63
src/slave/containerizer/mesos/provisioner/docker/puller.cpp f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2
src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp ad57d8592c5f9df5115b575855ed2e99a0597359
src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp PRE-CREATION
src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp PRE-CREATION
src/slave/containerizer/mesos/provisioner/docker/store.cpp bb02d650e16d45fcf337a7954f7a26143fb2c69f
src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73
src/slave/flags.cpp ed9b0b8313f5a5e53f3715af5300d9fcaa936df8
src/tests/containerizer/provisioner_docker_tests.cpp 8d90894410cd834edf49a2814d1b616718798fe8
Diff: https://reviews.apache.org/r/38580/diff/
Testing
-------
make check.
Thanks,
Jojy Varghese
Re: Review Request 38580: Added docker registry RemotePuller
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38580/
-----------------------------------------------------------
(Updated Nov. 5, 2015, 11:57 p.m.)
Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu.
Changes
-------
review comments
Repository: mesos
Description
-------
Integrated remote puller with store. Tests will follow.
Diffs (updated)
-----
src/CMakeLists.txt d107e329cc6887cd9d4ce3706dfc6ce6080d0289
src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db
src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 87d80026939a326bd1169f46906e36d6ef19f546
src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp f314f20d989ed0125e15d2a14d0f8e56c56976d5
src/slave/containerizer/mesos/provisioner/docker/puller.hpp 8010b8aa75a2fc2e4b537f915f9dca028e407b63
src/slave/containerizer/mesos/provisioner/docker/puller.cpp f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2
src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp ad57d8592c5f9df5115b575855ed2e99a0597359
src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c
src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp PRE-CREATION
src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp PRE-CREATION
src/slave/containerizer/mesos/provisioner/docker/store.cpp bb02d650e16d45fcf337a7954f7a26143fb2c69f
src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73
src/slave/flags.cpp ed9b0b8313f5a5e53f3715af5300d9fcaa936df8
src/tests/containerizer/provisioner_docker_tests.cpp 8d90894410cd834edf49a2814d1b616718798fe8
Diff: https://reviews.apache.org/r/38580/diff/
Testing
-------
make check.
Thanks,
Jojy Varghese
Re: Review Request 38580: Added docker registry RemotePuller
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38580/#review105325
-----------------------------------------------------------
src/tests/containerizer/provisioner_docker_tests.cpp (line 1191)
<https://reviews.apache.org/r/38580/#comment163853>
I dont think this API of read does the non-block. In fact i think it expects it and errors out if the fd passed in is blocking.
src/tests/containerizer/provisioner_docker_tests.cpp (line 1217)
<https://reviews.apache.org/r/38580/#comment163851>
Copying the tar buffer into output buffer before sending it out. We are simulating a blob layer sent from server.
- Jojy Varghese
On Nov. 5, 2015, 4:03 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38580/
> -----------------------------------------------------------
>
> (Updated Nov. 5, 2015, 4:03 p.m.)
>
>
> Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Integrated remote puller with store. Tests will follow.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt d107e329cc6887cd9d4ce3706dfc6ce6080d0289
> src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db
> src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 87d80026939a326bd1169f46906e36d6ef19f546
> src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp f314f20d989ed0125e15d2a14d0f8e56c56976d5
> src/slave/containerizer/mesos/provisioner/docker/puller.hpp 8010b8aa75a2fc2e4b537f915f9dca028e407b63
> src/slave/containerizer/mesos/provisioner/docker/puller.cpp f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2
> src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp ad57d8592c5f9df5115b575855ed2e99a0597359
> src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c
> src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp PRE-CREATION
> src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp PRE-CREATION
> src/slave/containerizer/mesos/provisioner/docker/store.cpp bb02d650e16d45fcf337a7954f7a26143fb2c69f
> src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73
> src/slave/flags.cpp ed9b0b8313f5a5e53f3715af5300d9fcaa936df8
> src/tests/containerizer/provisioner_docker_tests.cpp 8d90894410cd834edf49a2814d1b616718798fe8
>
> Diff: https://reviews.apache.org/r/38580/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 38580: Added docker registry RemotePuller
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38580/#review105306
-----------------------------------------------------------
src/tests/containerizer/provisioner_docker_tests.cpp (line 1001)
<https://reviews.apache.org/r/38580/#comment163824>
Please update the comment as we discussed in our meeting, to be more descriptive what you're actually testing.
src/tests/containerizer/provisioner_docker_tests.cpp (line 1004)
<https://reviews.apache.org/r/38580/#comment163823>
Let's move the setup_server and ASSERTS to the Setup method of RegistryClientTest class as we're doing it every time.
src/tests/containerizer/provisioner_docker_tests.cpp (line 1191)
<https://reviews.apache.org/r/38580/#comment163825>
io::read should already do os::nonblock, you don't need to do it agian.
src/tests/containerizer/provisioner_docker_tests.cpp (line 1217)
<https://reviews.apache.org/r/38580/#comment163826>
What are these for?
- Timothy Chen
On Nov. 5, 2015, 4:03 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38580/
> -----------------------------------------------------------
>
> (Updated Nov. 5, 2015, 4:03 p.m.)
>
>
> Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Integrated remote puller with store. Tests will follow.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt d107e329cc6887cd9d4ce3706dfc6ce6080d0289
> src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db
> src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 87d80026939a326bd1169f46906e36d6ef19f546
> src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp f314f20d989ed0125e15d2a14d0f8e56c56976d5
> src/slave/containerizer/mesos/provisioner/docker/puller.hpp 8010b8aa75a2fc2e4b537f915f9dca028e407b63
> src/slave/containerizer/mesos/provisioner/docker/puller.cpp f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2
> src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp ad57d8592c5f9df5115b575855ed2e99a0597359
> src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c
> src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp PRE-CREATION
> src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp PRE-CREATION
> src/slave/containerizer/mesos/provisioner/docker/store.cpp bb02d650e16d45fcf337a7954f7a26143fb2c69f
> src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73
> src/slave/flags.cpp ed9b0b8313f5a5e53f3715af5300d9fcaa936df8
> src/tests/containerizer/provisioner_docker_tests.cpp 8d90894410cd834edf49a2814d1b616718798fe8
>
> Diff: https://reviews.apache.org/r/38580/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 38580: Added docker registry RemotePuller
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38580/
-----------------------------------------------------------
(Updated Nov. 5, 2015, 4:03 p.m.)
Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu.
Changes
-------
rebased
Repository: mesos
Description
-------
Integrated remote puller with store. Tests will follow.
Diffs (updated)
-----
src/CMakeLists.txt d107e329cc6887cd9d4ce3706dfc6ce6080d0289
src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db
src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 87d80026939a326bd1169f46906e36d6ef19f546
src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp f314f20d989ed0125e15d2a14d0f8e56c56976d5
src/slave/containerizer/mesos/provisioner/docker/puller.hpp 8010b8aa75a2fc2e4b537f915f9dca028e407b63
src/slave/containerizer/mesos/provisioner/docker/puller.cpp f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2
src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp ad57d8592c5f9df5115b575855ed2e99a0597359
src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c
src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp PRE-CREATION
src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp PRE-CREATION
src/slave/containerizer/mesos/provisioner/docker/store.cpp bb02d650e16d45fcf337a7954f7a26143fb2c69f
src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73
src/slave/flags.cpp ed9b0b8313f5a5e53f3715af5300d9fcaa936df8
src/tests/containerizer/provisioner_docker_tests.cpp 8d90894410cd834edf49a2814d1b616718798fe8
Diff: https://reviews.apache.org/r/38580/diff/
Testing
-------
make check.
Thanks,
Jojy Varghese
Re: Review Request 38580: Added docker registry RemotePuller
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38580/
-----------------------------------------------------------
(Updated Nov. 4, 2015, 11:27 p.m.)
Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu.
Changes
-------
review comments addressed.
Repository: mesos
Description
-------
Integrated remote puller with store. Tests will follow.
Diffs (updated)
-----
src/CMakeLists.txt d107e329cc6887cd9d4ce3706dfc6ce6080d0289
src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db
src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 87d80026939a326bd1169f46906e36d6ef19f546
src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp f314f20d989ed0125e15d2a14d0f8e56c56976d5
src/slave/containerizer/mesos/provisioner/docker/puller.hpp 8010b8aa75a2fc2e4b537f915f9dca028e407b63
src/slave/containerizer/mesos/provisioner/docker/puller.cpp f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2
src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp ad57d8592c5f9df5115b575855ed2e99a0597359
src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c
src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp PRE-CREATION
src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp PRE-CREATION
src/slave/containerizer/mesos/provisioner/docker/store.cpp bb02d650e16d45fcf337a7954f7a26143fb2c69f
src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73
src/slave/flags.cpp ed9b0b8313f5a5e53f3715af5300d9fcaa936df8
src/tests/containerizer/provisioner_docker_tests.cpp 8d90894410cd834edf49a2814d1b616718798fe8
Diff: https://reviews.apache.org/r/38580/diff/
Testing
-------
make check.
Thanks,
Jojy Varghese
Re: Review Request 38580: Added docker registry RemotePuller
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38580/#review105167
-----------------------------------------------------------
src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp (line 129)
<https://reviews.apache.org/r/38580/#comment163587>
Then will the timeout property be that of the store?
- Jojy Varghese
On Nov. 4, 2015, 6:09 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38580/
> -----------------------------------------------------------
>
> (Updated Nov. 4, 2015, 6:09 p.m.)
>
>
> Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Integrated remote puller with store. Tests will follow.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt d107e329cc6887cd9d4ce3706dfc6ce6080d0289
> src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db
> src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 87d80026939a326bd1169f46906e36d6ef19f546
> src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp f314f20d989ed0125e15d2a14d0f8e56c56976d5
> src/slave/containerizer/mesos/provisioner/docker/puller.hpp 8010b8aa75a2fc2e4b537f915f9dca028e407b63
> src/slave/containerizer/mesos/provisioner/docker/puller.cpp f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2
> src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp ad57d8592c5f9df5115b575855ed2e99a0597359
> src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c
> src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp PRE-CREATION
> src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp PRE-CREATION
> src/slave/containerizer/mesos/provisioner/docker/store.cpp bb02d650e16d45fcf337a7954f7a26143fb2c69f
> src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73
> src/slave/flags.cpp ed9b0b8313f5a5e53f3715af5300d9fcaa936df8
> src/tests/containerizer/provisioner_docker_tests.cpp 8d90894410cd834edf49a2814d1b616718798fe8
>
> Diff: https://reviews.apache.org/r/38580/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 38580: Added docker registry RemotePuller
Posted by Timothy Chen <tn...@apache.org>.
> On Nov. 4, 2015, 10:54 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp, line 373
> > <https://reviews.apache.org/r/38580/diff/19/?file=1115760#file1115760line373>
> >
> > Are you suggesting moving lines 373 and 374 into getRemoteNameFromLocalName method?
Yes, and let getRemoteNameFromLocalName return a Image::Name instead of a string.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38580/#review105166
-----------------------------------------------------------
On Nov. 4, 2015, 6:09 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38580/
> -----------------------------------------------------------
>
> (Updated Nov. 4, 2015, 6:09 p.m.)
>
>
> Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Integrated remote puller with store. Tests will follow.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt d107e329cc6887cd9d4ce3706dfc6ce6080d0289
> src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db
> src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 87d80026939a326bd1169f46906e36d6ef19f546
> src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp f314f20d989ed0125e15d2a14d0f8e56c56976d5
> src/slave/containerizer/mesos/provisioner/docker/puller.hpp 8010b8aa75a2fc2e4b537f915f9dca028e407b63
> src/slave/containerizer/mesos/provisioner/docker/puller.cpp f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2
> src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp ad57d8592c5f9df5115b575855ed2e99a0597359
> src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c
> src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp PRE-CREATION
> src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp PRE-CREATION
> src/slave/containerizer/mesos/provisioner/docker/store.cpp bb02d650e16d45fcf337a7954f7a26143fb2c69f
> src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73
> src/slave/flags.cpp ed9b0b8313f5a5e53f3715af5300d9fcaa936df8
> src/tests/containerizer/provisioner_docker_tests.cpp 8d90894410cd834edf49a2814d1b616718798fe8
>
> Diff: https://reviews.apache.org/r/38580/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 38580: Added docker registry RemotePuller
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38580/#review105166
-----------------------------------------------------------
src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp (line 373)
<https://reviews.apache.org/r/38580/#comment163586>
Are you suggesting moving lines 373 and 374 into getRemoteNameFromLocalName method?
- Jojy Varghese
On Nov. 4, 2015, 6:09 p.m., Jojy Varghese wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38580/
> -----------------------------------------------------------
>
> (Updated Nov. 4, 2015, 6:09 p.m.)
>
>
> Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Integrated remote puller with store. Tests will follow.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt d107e329cc6887cd9d4ce3706dfc6ce6080d0289
> src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db
> src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 87d80026939a326bd1169f46906e36d6ef19f546
> src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp f314f20d989ed0125e15d2a14d0f8e56c56976d5
> src/slave/containerizer/mesos/provisioner/docker/puller.hpp 8010b8aa75a2fc2e4b537f915f9dca028e407b63
> src/slave/containerizer/mesos/provisioner/docker/puller.cpp f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2
> src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp ad57d8592c5f9df5115b575855ed2e99a0597359
> src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c
> src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp PRE-CREATION
> src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp PRE-CREATION
> src/slave/containerizer/mesos/provisioner/docker/store.cpp bb02d650e16d45fcf337a7954f7a26143fb2c69f
> src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73
> src/slave/flags.cpp ed9b0b8313f5a5e53f3715af5300d9fcaa936df8
> src/tests/containerizer/provisioner_docker_tests.cpp 8d90894410cd834edf49a2814d1b616718798fe8
>
> Diff: https://reviews.apache.org/r/38580/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jojy Varghese
>
>
Re: Review Request 38580: Added docker registry RemotePuller
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38580/
-----------------------------------------------------------
(Updated Nov. 4, 2015, 6:09 p.m.)
Review request for mesos, Jie Yu, Timothy Chen, and Jiang Yan Xu.
Changes
-------
added discard semantics
Repository: mesos
Description
-------
Integrated remote puller with store. Tests will follow.
Diffs (updated)
-----
src/CMakeLists.txt d107e329cc6887cd9d4ce3706dfc6ce6080d0289
src/Makefile.am d6eb302f0e812a777f51f421deef89140871a1db
src/slave/containerizer/mesos/provisioner/docker/local_puller.hpp 87d80026939a326bd1169f46906e36d6ef19f546
src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp f314f20d989ed0125e15d2a14d0f8e56c56976d5
src/slave/containerizer/mesos/provisioner/docker/puller.hpp 8010b8aa75a2fc2e4b537f915f9dca028e407b63
src/slave/containerizer/mesos/provisioner/docker/puller.cpp f61f9e5e9e97a771a03b75ff17cfcd3e3ecbc9b2
src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp ad57d8592c5f9df5115b575855ed2e99a0597359
src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp e4d2c22cf6627c1c76ebafeeb84b2bbf6b8c238c
src/slave/containerizer/mesos/provisioner/docker/remote_puller.hpp PRE-CREATION
src/slave/containerizer/mesos/provisioner/docker/remote_puller.cpp PRE-CREATION
src/slave/containerizer/mesos/provisioner/docker/store.cpp bb02d650e16d45fcf337a7954f7a26143fb2c69f
src/slave/flags.hpp 3e93b52a5874f52361d5a9c685499a7032014a73
src/slave/flags.cpp ed9b0b8313f5a5e53f3715af5300d9fcaa936df8
src/tests/containerizer/provisioner_docker_tests.cpp 8d90894410cd834edf49a2814d1b616718798fe8
Diff: https://reviews.apache.org/r/38580/diff/
Testing
-------
make check.
Thanks,
Jojy Varghese