You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2015/12/11 02:35:11 UTC
Review Request 41235: Cleaned up the untar method in docker puller.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41235/
-----------------------------------------------------------
Review request for mesos, Ben Mahler, Gilbert Song, Jojy Varghese, and Timothy Chen.
Repository: mesos
Description
-------
Cleaned up the untar method in docker puller.
The extra promise is not needed.
Diffs
-----
src/slave/containerizer/mesos/provisioner/docker/puller.cpp 4aa4a9c4074d96c30c3bceea59d071feeecae2ea
Diff: https://reviews.apache.org/r/41235/diff/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 41235: Cleaned up the untar method in docker
puller.
Posted by Gilbert Song <so...@gmail.com>.
> On Dec. 11, 2015, 10:06 a.m., Gilbert Song wrote:
> > Ship It!
Tested.
Checked that logic does not change, but more clearer.
- Gilbert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41235/#review109973
-----------------------------------------------------------
On Dec. 10, 2015, 5:35 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41235/
> -----------------------------------------------------------
>
> (Updated Dec. 10, 2015, 5:35 p.m.)
>
>
> Review request for mesos, Ben Mahler, Gilbert Song, Jojy Varghese, and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Cleaned up the untar method in docker puller.
>
> The extra promise is not needed.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/provisioner/docker/puller.cpp 4aa4a9c4074d96c30c3bceea59d071feeecae2ea
>
> Diff: https://reviews.apache.org/r/41235/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 41235: Cleaned up the untar method in docker
puller.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41235/#review109973
-----------------------------------------------------------
Ship it!
Ship It!
- Gilbert Song
On Dec. 10, 2015, 5:35 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41235/
> -----------------------------------------------------------
>
> (Updated Dec. 10, 2015, 5:35 p.m.)
>
>
> Review request for mesos, Ben Mahler, Gilbert Song, Jojy Varghese, and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Cleaned up the untar method in docker puller.
>
> The extra promise is not needed.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/provisioner/docker/puller.cpp 4aa4a9c4074d96c30c3bceea59d071feeecae2ea
>
> Diff: https://reviews.apache.org/r/41235/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 41235: Cleaned up the untar method in docker
puller.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41235/
-----------------------------------------------------------
(Updated Dec. 11, 2015, 7:51 p.m.)
Review request for mesos, Ben Mahler, Gilbert Song, Jojy Varghese, and Timothy Chen.
Changes
-------
Addressed review comments.
Repository: mesos
Description
-------
Cleaned up the untar method in docker puller.
The extra promise is not needed.
Diffs (updated)
-----
src/slave/containerizer/mesos/provisioner/docker/puller.cpp 4aa4a9c4074d96c30c3bceea59d071feeecae2ea
Diff: https://reviews.apache.org/r/41235/diff/
Testing
-------
make check
Thanks,
Jie Yu
Re: Review Request 41235: Cleaned up the untar method in docker
puller.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On Dec. 11, 2015, 7 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/mesos/provisioner/docker/puller.cpp, line 96
> > <https://reviews.apache.org/r/41235/diff/1/?file=1159660#file1159660line96>
> >
> > The pattern I have seen in the code is that we dont log failures and it pops up the stack and at the place where they are logged, it loses context.
>
> Jie Yu wrote:
> If that's the case, that's something we need to fix:)
indeed :)
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41235/#review109986
-----------------------------------------------------------
On Dec. 11, 2015, 1:35 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41235/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2015, 1:35 a.m.)
>
>
> Review request for mesos, Ben Mahler, Gilbert Song, Jojy Varghese, and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Cleaned up the untar method in docker puller.
>
> The extra promise is not needed.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/provisioner/docker/puller.cpp 4aa4a9c4074d96c30c3bceea59d071feeecae2ea
>
> Diff: https://reviews.apache.org/r/41235/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 41235: Cleaned up the untar method in docker
puller.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On Dec. 11, 2015, 7 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/mesos/provisioner/docker/puller.cpp, line 117
> > <https://reviews.apache.org/r/41235/diff/1/?file=1159660#file1159660line117>
> >
> > If the future is not expected to be ready when we reach here, should we do a .then on it instead of expecting it to be ready and returning failure?
>
> Jie Yu wrote:
> How do you use a .then here?
>
> The main issue with the original code is that we start io::read after the process has finished. This is potentially problematic because the subprocess might write a lot of stuff to stderr and be blocked due to the fact that the pipe is full. We have to start the read early, instead of waiting for the subprocess to terminate first.
Since the read future is not guaranteed to be ready by the time we reach here, i was thinking that we should add a continuation to that future to read the output when its ready (or for any event). Isnt it? Also, if the pipe is waiting to be flushed, then dont we have to retry read in a loop?
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41235/#review109986
-----------------------------------------------------------
On Dec. 11, 2015, 7:51 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41235/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2015, 7:51 p.m.)
>
>
> Review request for mesos, Ben Mahler, Gilbert Song, Jojy Varghese, and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Cleaned up the untar method in docker puller.
>
> The extra promise is not needed.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/provisioner/docker/puller.cpp 4aa4a9c4074d96c30c3bceea59d071feeecae2ea
>
> Diff: https://reviews.apache.org/r/41235/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 41235: Cleaned up the untar method in docker
puller.
Posted by Jie Yu <yu...@gmail.com>.
> On Dec. 11, 2015, 7 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/mesos/provisioner/docker/puller.cpp, line 96
> > <https://reviews.apache.org/r/41235/diff/1/?file=1159660#file1159660line96>
> >
> > The pattern I have seen in the code is that we dont log failures and it pops up the stack and at the place where they are logged, it loses context.
If that's the case, that's something we need to fix:)
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41235/#review109986
-----------------------------------------------------------
On Dec. 11, 2015, 1:35 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41235/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2015, 1:35 a.m.)
>
>
> Review request for mesos, Ben Mahler, Gilbert Song, Jojy Varghese, and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Cleaned up the untar method in docker puller.
>
> The extra promise is not needed.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/provisioner/docker/puller.cpp 4aa4a9c4074d96c30c3bceea59d071feeecae2ea
>
> Diff: https://reviews.apache.org/r/41235/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 41235: Cleaned up the untar method in docker
puller.
Posted by Jie Yu <yu...@gmail.com>.
> On Dec. 11, 2015, 7 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/mesos/provisioner/docker/puller.cpp, line 117
> > <https://reviews.apache.org/r/41235/diff/1/?file=1159660#file1159660line117>
> >
> > If the future is not expected to be ready when we reach here, should we do a .then on it instead of expecting it to be ready and returning failure?
How do you use a .then here?
The main issue with the original code is that we start io::read after the process has finished. This is potentially problematic because the subprocess might write a lot of stuff to stderr and be blocked due to the fact that the pipe is full. We have to start the read early, instead of waiting for the subprocess to terminate first.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41235/#review109986
-----------------------------------------------------------
On Dec. 11, 2015, 1:35 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41235/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2015, 1:35 a.m.)
>
>
> Review request for mesos, Ben Mahler, Gilbert Song, Jojy Varghese, and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Cleaned up the untar method in docker puller.
>
> The extra promise is not needed.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/provisioner/docker/puller.cpp 4aa4a9c4074d96c30c3bceea59d071feeecae2ea
>
> Diff: https://reviews.apache.org/r/41235/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 41235: Cleaned up the untar method in docker
puller.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> On Dec. 11, 2015, 7 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/mesos/provisioner/docker/puller.cpp, line 117
> > <https://reviews.apache.org/r/41235/diff/1/?file=1159660#file1159660line117>
> >
> > If the future is not expected to be ready when we reach here, should we do a .then on it instead of expecting it to be ready and returning failure?
>
> Jie Yu wrote:
> How do you use a .then here?
>
> The main issue with the original code is that we start io::read after the process has finished. This is potentially problematic because the subprocess might write a lot of stuff to stderr and be blocked due to the fact that the pipe is full. We have to start the read early, instead of waiting for the subprocess to terminate first.
>
> Jojy Varghese wrote:
> Since the read future is not guaranteed to be ready by the time we reach here, i was thinking that we should add a continuation to that future to read the output when its ready (or for any event). Isnt it? Also, if the pipe is waiting to be flushed, then dont we have to retry read in a loop?
>
> Jie Yu wrote:
> > Since the read future is not guaranteed to be ready by the time we reach here
>
> It will be in one of the following state once 'await' finished: READY, FAILED, DISCARDED. Why do you need a then here?
>
> io::read continuesly and asynchrously read from stderr and will stop if EOF is reached or some error occured. 'await' won't become ready if io::read hasn't finished.
Gotcha. I didnt know "await" details. I know something now :). Thanks Jie.
- Jojy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41235/#review109986
-----------------------------------------------------------
On Dec. 11, 2015, 7:51 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41235/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2015, 7:51 p.m.)
>
>
> Review request for mesos, Ben Mahler, Gilbert Song, Jojy Varghese, and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Cleaned up the untar method in docker puller.
>
> The extra promise is not needed.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/provisioner/docker/puller.cpp 4aa4a9c4074d96c30c3bceea59d071feeecae2ea
>
> Diff: https://reviews.apache.org/r/41235/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 41235: Cleaned up the untar method in docker
puller.
Posted by Jie Yu <yu...@gmail.com>.
> On Dec. 11, 2015, 7 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/mesos/provisioner/docker/puller.cpp, line 117
> > <https://reviews.apache.org/r/41235/diff/1/?file=1159660#file1159660line117>
> >
> > If the future is not expected to be ready when we reach here, should we do a .then on it instead of expecting it to be ready and returning failure?
>
> Jie Yu wrote:
> How do you use a .then here?
>
> The main issue with the original code is that we start io::read after the process has finished. This is potentially problematic because the subprocess might write a lot of stuff to stderr and be blocked due to the fact that the pipe is full. We have to start the read early, instead of waiting for the subprocess to terminate first.
>
> Jojy Varghese wrote:
> Since the read future is not guaranteed to be ready by the time we reach here, i was thinking that we should add a continuation to that future to read the output when its ready (or for any event). Isnt it? Also, if the pipe is waiting to be flushed, then dont we have to retry read in a loop?
> Since the read future is not guaranteed to be ready by the time we reach here
It will be in one of the following state once 'await' finished: READY, FAILED, DISCARDED. Why do you need a then here?
io::read continuesly and asynchrously read from stderr and will stop if EOF is reached or some error occured. 'await' won't become ready if io::read hasn't finished.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41235/#review109986
-----------------------------------------------------------
On Dec. 11, 2015, 7:51 p.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41235/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2015, 7:51 p.m.)
>
>
> Review request for mesos, Ben Mahler, Gilbert Song, Jojy Varghese, and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Cleaned up the untar method in docker puller.
>
> The extra promise is not needed.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/provisioner/docker/puller.cpp 4aa4a9c4074d96c30c3bceea59d071feeecae2ea
>
> Diff: https://reviews.apache.org/r/41235/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 41235: Cleaned up the untar method in docker
puller.
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41235/#review109986
-----------------------------------------------------------
src/slave/containerizer/mesos/provisioner/docker/puller.cpp
<https://reviews.apache.org/r/41235/#comment169728>
The pattern I have seen in the code is that we dont log failures and it pops up the stack and at the place where they are logged, it loses context.
src/slave/containerizer/mesos/provisioner/docker/puller.cpp (line 107)
<https://reviews.apache.org/r/41235/#comment169731>
If the future is not expected to be ready when we reach here, should we do a .then on it instead of expecting it to be ready and returning failure?
- Jojy Varghese
On Dec. 11, 2015, 1:35 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41235/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2015, 1:35 a.m.)
>
>
> Review request for mesos, Ben Mahler, Gilbert Song, Jojy Varghese, and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Cleaned up the untar method in docker puller.
>
> The extra promise is not needed.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/provisioner/docker/puller.cpp 4aa4a9c4074d96c30c3bceea59d071feeecae2ea
>
> Diff: https://reviews.apache.org/r/41235/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 41235: Cleaned up the untar method in docker
puller.
Posted by Jie Yu <yu...@gmail.com>.
> On Dec. 11, 2015, 6:42 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/mesos/provisioner/docker/puller.cpp, line 117
> > <https://reviews.apache.org/r/41235/diff/1/?file=1159660#file1159660line117>
> >
> > Are we always expecting the future to be ready when we get here?
Not really. that's the subtle difference between 'await' and 'collect'.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41235/#review109981
-----------------------------------------------------------
On Dec. 11, 2015, 1:35 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41235/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2015, 1:35 a.m.)
>
>
> Review request for mesos, Ben Mahler, Gilbert Song, Jojy Varghese, and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Cleaned up the untar method in docker puller.
>
> The extra promise is not needed.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/provisioner/docker/puller.cpp 4aa4a9c4074d96c30c3bceea59d071feeecae2ea
>
> Diff: https://reviews.apache.org/r/41235/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 41235: Cleaned up the untar method in docker
puller.
Posted by Jie Yu <yu...@gmail.com>.
> On Dec. 11, 2015, 6:42 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/mesos/provisioner/docker/puller.cpp, line 96
> > <https://reviews.apache.org/r/41235/diff/1/?file=1159660#file1159660line96>
> >
> > Wondering why we removed error context like the name of the file. Wouldnt that be great debugging information?
Because the caller is responsible for logging the name of the file. This is a common protocol we use: if the caller has the information, always relying on caller to log that information to avoid double logging of that information.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41235/#review109981
-----------------------------------------------------------
On Dec. 11, 2015, 1:35 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41235/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2015, 1:35 a.m.)
>
>
> Review request for mesos, Ben Mahler, Gilbert Song, Jojy Varghese, and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Cleaned up the untar method in docker puller.
>
> The extra promise is not needed.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/provisioner/docker/puller.cpp 4aa4a9c4074d96c30c3bceea59d071feeecae2ea
>
> Diff: https://reviews.apache.org/r/41235/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 41235: Cleaned up the untar method in docker
puller.
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41235/#review109981
-----------------------------------------------------------
src/slave/containerizer/mesos/provisioner/docker/puller.cpp
<https://reviews.apache.org/r/41235/#comment169720>
Wondering why we removed error context like the name of the file. Wouldnt that be great debugging information?
src/slave/containerizer/mesos/provisioner/docker/puller.cpp (line 107)
<https://reviews.apache.org/r/41235/#comment169723>
Are we always expecting the future to be ready when we get here?
- Jojy Varghese
On Dec. 11, 2015, 1:35 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41235/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2015, 1:35 a.m.)
>
>
> Review request for mesos, Ben Mahler, Gilbert Song, Jojy Varghese, and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Cleaned up the untar method in docker puller.
>
> The extra promise is not needed.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/provisioner/docker/puller.cpp 4aa4a9c4074d96c30c3bceea59d071feeecae2ea
>
> Diff: https://reviews.apache.org/r/41235/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 41235: Cleaned up the untar method in docker
puller.
Posted by Jie Yu <yu...@gmail.com>.
> On Dec. 11, 2015, 6:33 p.m., Ben Mahler wrote:
> > src/slave/containerizer/mesos/provisioner/docker/puller.cpp, line 37
> > <https://reviews.apache.org/r/41235/diff/1/?file=1159660#file1159660line37>
> >
> > We've been trying to eliminate these in favor of non-namespace using statements and namespace aliases, could you remove it?
Got it. Will revert this.
> On Dec. 11, 2015, 6:33 p.m., Ben Mahler wrote:
> > src/slave/containerizer/mesos/provisioner/docker/puller.cpp, lines 103-105
> > <https://reviews.apache.org/r/41235/diff/1/?file=1159660#file1159660line103>
> >
> > No need for the outer Future + CHECK_READY if you're using .then here. Something I'm missing?
good catch. wish compiler can catch such errors:)
> On Dec. 11, 2015, 6:33 p.m., Ben Mahler wrote:
> > src/slave/containerizer/mesos/provisioner/docker/puller.cpp, lines 147-150
> > <https://reviews.apache.org/r/41235/diff/1/?file=1159660#file1159660line147>
> >
> > Hm.. it would be great to print the status using WSTRINGIFY on the status as we do throughout the rest of the code, any reason not to?
Just got to know this function. Thanks Ben!
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41235/#review109978
-----------------------------------------------------------
On Dec. 11, 2015, 1:35 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41235/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2015, 1:35 a.m.)
>
>
> Review request for mesos, Ben Mahler, Gilbert Song, Jojy Varghese, and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Cleaned up the untar method in docker puller.
>
> The extra promise is not needed.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/provisioner/docker/puller.cpp 4aa4a9c4074d96c30c3bceea59d071feeecae2ea
>
> Diff: https://reviews.apache.org/r/41235/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>
Re: Review Request 41235: Cleaned up the untar method in docker
puller.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41235/#review109978
-----------------------------------------------------------
Ship it!
Thanks Jie!!
src/slave/containerizer/mesos/provisioner/docker/puller.cpp (line 36)
<https://reviews.apache.org/r/41235/#comment169714>
We've been trying to eliminate these in favor of non-namespace using statements and namespace aliases, could you remove it?
src/slave/containerizer/mesos/provisioner/docker/puller.cpp (lines 94 - 96)
<https://reviews.apache.org/r/41235/#comment169715>
No need for the outer Future + CHECK_READY if you're using .then here. Something I'm missing?
src/slave/containerizer/mesos/provisioner/docker/puller.cpp (lines 118 - 121)
<https://reviews.apache.org/r/41235/#comment169716>
Hm.. it would be great to print the status using WSTRINGIFY on the status as we do throughout the rest of the code, any reason not to?
- Ben Mahler
On Dec. 11, 2015, 1:35 a.m., Jie Yu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41235/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2015, 1:35 a.m.)
>
>
> Review request for mesos, Ben Mahler, Gilbert Song, Jojy Varghese, and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Cleaned up the untar method in docker puller.
>
> The extra promise is not needed.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/provisioner/docker/puller.cpp 4aa4a9c4074d96c30c3bceea59d071feeecae2ea
>
> Diff: https://reviews.apache.org/r/41235/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jie Yu
>
>