Re: Review Request 40285: Changed untar process to pipe STDOUT and STDERR.

2015-11-16 Thread Mesos ReviewBot

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


Bad patch!

Reviews applied: [40285]

Failed command: ./support/apply-review.sh -n -r 40285

Error:
 2015-11-16 21:20:39 URL:https://reviews.apache.org/r/40285/diff/raw/ 
[2413/2413] -> "40285.patch" [1]
error: patch failed: 
src/slave/containerizer/mesos/provisioner/docker/puller.cpp:18
error: src/slave/containerizer/mesos/provisioner/docker/puller.cpp: patch does 
not apply
Failed to apply patch

- Mesos ReviewBot


On Nov. 16, 2015, 8:49 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40285/
> ---
> 
> (Updated Nov. 16, 2015, 8:49 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> By piping stdout and stderr to logs, it would be easier to  debug problems 
> with untar.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
> 13f5e2877f4d7951e79ba07073a42848217604b3 
> 
> Diff: https://reviews.apache.org/r/40285/diff/
> 
> 
> Testing
> ---
> 
> make check;
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>



Re: Review Request 40285: Changed untar process to pipe STDOUT and STDERR.

2015-11-16 Thread Jojy Varghese

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

(Updated Nov. 16, 2015, 8:49 p.m.)


Review request for mesos and Timothy Chen.


Changes
---

added stderr to Failure report.


Repository: mesos


Description
---

By piping stdout and stderr to logs, it would be easier to  debug problems with 
untar.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
13f5e2877f4d7951e79ba07073a42848217604b3 

Diff: https://reviews.apache.org/r/40285/diff/


Testing
---

make check;


Thanks,

Jojy Varghese



Re: Review Request 40285: Changed untar process to pipe STDOUT and STDERR.

2015-11-16 Thread Timothy Chen

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



src/slave/containerizer/mesos/provisioner/docker/puller.cpp (line 97)


As discussed, let's not created shared_ptr and update it in a .then block.

Let's return a .then with a Future parameter and check in the .then block 
what the result is.


- Timothy Chen


On Nov. 16, 2015, 8:49 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40285/
> ---
> 
> (Updated Nov. 16, 2015, 8:49 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> By piping stdout and stderr to logs, it would be easier to  debug problems 
> with untar.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
> 13f5e2877f4d7951e79ba07073a42848217604b3 
> 
> Diff: https://reviews.apache.org/r/40285/diff/
> 
> 
> Testing
> ---
> 
> make check;
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>



Re: Review Request 40285: Changed untar process to pipe STDOUT and STDERR.

2015-11-13 Thread Mesos ReviewBot

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


Patch looks great!

Reviews applied: [40285]

All tests passed.

- Mesos ReviewBot


On Nov. 13, 2015, 8:13 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40285/
> ---
> 
> (Updated Nov. 13, 2015, 8:13 a.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> By piping stdout and stderr to logs, it would be easier to  debug problems 
> with untar.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
> 13f5e2877f4d7951e79ba07073a42848217604b3 
> 
> Diff: https://reviews.apache.org/r/40285/diff/
> 
> 
> Testing
> ---
> 
> make check;
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>



Re: Review Request 40285: Changed untar process to pipe STDOUT and STDERR.

2015-11-13 Thread Timothy Chen

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



src/slave/containerizer/mesos/provisioner/docker/puller.cpp (line 78)


This would then print the whole untar structure on every layer and every 
local image.

What we should instead is that when untar fails we should print the stderr 
message from the pipe instead.


- Timothy Chen


On Nov. 13, 2015, 8:13 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40285/
> ---
> 
> (Updated Nov. 13, 2015, 8:13 a.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> By piping stdout and stderr to logs, it would be easier to  debug problems 
> with untar.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
> 13f5e2877f4d7951e79ba07073a42848217604b3 
> 
> Diff: https://reviews.apache.org/r/40285/diff/
> 
> 
> Testing
> ---
> 
> make check;
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>



Re: Review Request 40285: Changed untar process to pipe STDOUT and STDERR.

2015-11-13 Thread Jojy Varghese


> On Nov. 13, 2015, 4:49 p.m., Timothy Chen wrote:
> > src/slave/containerizer/mesos/provisioner/docker/puller.cpp, line 78
> > 
> >
> > This would then print the whole untar structure on every layer and 
> > every local image.
> > 
> > What we should instead is that when untar fails we should print the 
> > stderr message from the pipe instead.

I dont think it will print the entire file list since -v flag is not provided 
to the tar. Since we are piping stderr to the mesos-slave's stderr, the idea 
was that we will get the error logs at slave's stderr.


- Jojy


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


On Nov. 13, 2015, 8:13 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40285/
> ---
> 
> (Updated Nov. 13, 2015, 8:13 a.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> By piping stdout and stderr to logs, it would be easier to  debug problems 
> with untar.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
> 13f5e2877f4d7951e79ba07073a42848217604b3 
> 
> Diff: https://reviews.apache.org/r/40285/diff/
> 
> 
> Testing
> ---
> 
> make check;
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>



Re: Review Request 40285: Changed untar process to pipe STDOUT and STDERR.

2015-11-13 Thread Timothy Chen

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



src/slave/containerizer/mesos/provisioner/docker/puller.cpp (line 78)


But the user then have to search the log around to see what's the tar's 
stderr? there isn't any message around this too so it's going to be scattered 
and probably be seperated as well.

If tar gave you "Not enough permissions" on stderr, how does a user know 
where to find that and also correlate that this message is from untar?

I think the better way is to read the stderr into here and return a Failure 
that can be correlated with the untar Future, and the caller can do:

"Failed to untar image x: Not enough permissions"

Let me know if you think otherwise.


- Timothy Chen


On Nov. 13, 2015, 8:13 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40285/
> ---
> 
> (Updated Nov. 13, 2015, 8:13 a.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> By piping stdout and stderr to logs, it would be easier to  debug problems 
> with untar.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
> 13f5e2877f4d7951e79ba07073a42848217604b3 
> 
> Diff: https://reviews.apache.org/r/40285/diff/
> 
> 
> Testing
> ---
> 
> make check;
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>



Re: Review Request 40285: Changed untar process to pipe STDOUT and STDERR.

2015-11-13 Thread Timothy Chen


> On Nov. 13, 2015, 4:49 p.m., Timothy Chen wrote:
> > src/slave/containerizer/mesos/provisioner/docker/puller.cpp, line 78
> > 
> >
> > This would then print the whole untar structure on every layer and 
> > every local image.
> > 
> > What we should instead is that when untar fails we should print the 
> > stderr message from the pipe instead.
> 
> Jojy Varghese wrote:
> I dont think it will print the entire file list since -v flag is not 
> provided to the tar. Since we are piping stderr to the mesos-slave's stderr, 
> the idea was that we will get the error logs at slave's stderr.

But the user then have to search the log around to see what's the tar's stderr? 
there isn't any message around this too so it's going to be scattered and 
probably be seperated as well.

If tar gave you "Not enough permissions" on stderr, how does a user know where 
to find that and also correlate that this message is from untar?

I think the better way is to read the stderr into here and return a Failure 
that can be correlated with the untar Future, and the caller can do:

"Failed to untar image x: Not enough permissions"

Let me know if you think otherwise.


- Timothy


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


On Nov. 13, 2015, 8:13 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40285/
> ---
> 
> (Updated Nov. 13, 2015, 8:13 a.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> By piping stdout and stderr to logs, it would be easier to  debug problems 
> with untar.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
> 13f5e2877f4d7951e79ba07073a42848217604b3 
> 
> Diff: https://reviews.apache.org/r/40285/diff/
> 
> 
> Testing
> ---
> 
> make check;
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>



Re: Review Request 40285: Changed untar process to pipe STDOUT and STDERR.

2015-11-13 Thread Jojy Varghese


> On Nov. 13, 2015, 5:31 p.m., Timothy Chen wrote:
> > src/slave/containerizer/mesos/provisioner/docker/puller.cpp, line 78
> > 
> >
> > But the user then have to search the log around to see what's the tar's 
> > stderr? there isn't any message around this too so it's going to be 
> > scattered and probably be seperated as well.
> > 
> > If tar gave you "Not enough permissions" on stderr, how does a user 
> > know where to find that and also correlate that this message is from untar?
> > 
> > I think the better way is to read the stderr into here and return a 
> > Failure that can be correlated with the untar Future, and the caller can do:
> > 
> > "Failed to untar image x: Not enough permissions"
> > 
> > Let me know if you think otherwise.

Makes sense. How do i read the stderr from the Subprocess ?


- Jojy


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


On Nov. 13, 2015, 8:13 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40285/
> ---
> 
> (Updated Nov. 13, 2015, 8:13 a.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> By piping stdout and stderr to logs, it would be easier to  debug problems 
> with untar.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
> 13f5e2877f4d7951e79ba07073a42848217604b3 
> 
> Diff: https://reviews.apache.org/r/40285/diff/
> 
> 
> Testing
> ---
> 
> make check;
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>