Re: Review Request 39338: Added code that appends the fetcher log to the agent log upon fetcher failure.

2015-10-16 Thread Ben Mahler 


> On Oct. 16, 2015, 7:29 a.m., Adam B wrote:
> > src/slave/containerizer/fetcher.cpp, line 794
> > 
> >
> > What's this string parameter that you're ignoring? If it's the Failure 
> > message, I'd think you'd want to log that too.
> > 
> > Should this lambda return a Future too? Otherwise, how will it 
> > run the following onAny cleanup below?
> 
> Bernd Mathiske wrote:
> onFailed() takes a parameter of this type:
> 
>   typedef lambda::function FailedCallback;
>   
> The string parameter of this callback receives the error message from the 
> failed future as argument when onFailed is called. The latter is declared 
> this way:
> 
>   const Future& onFailed(FailedCallback&& callback) const;
> 
> The future you refer to is passed through every onFailed call itself, not 
> through its callback, which must return void.
> 
> Bernd Mathiske wrote:
> The failure message has already been logged at this point.
> 
> Adam B wrote:
> Ok, that makes more sense to me. Just trying to figure out what happens 
> to the Failure message if you're ignoring it in onFailed(). So onFailed() 
> automatically passes on the (failed) Future to the onAny() below, and after 
> that it becomes the final object in the `return fetcherSubprocess...` 
> statement on line 779. Then whatever called FetcherProcess::Run() waits on 
> that future and hopefully logs the Failure message somewhere? So will you see 
> the "Begin fetcher log..." before "Failed to fetch all URIs for container..."?

It should compile without needing to have 'const string&' as an argument, was 
that not the case bernd?


- Ben


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


On Oct. 16, 2015, 8:56 a.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39338/
> ---
> 
> (Updated Oct. 16, 2015, 8:56 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-3743
> https://issues.apache.org/jira/browse/MESOS-3743
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added an onFailed() clause to the inspection of the fetcher subprocess run. 
> This clause copies the fetcher log from /stderr and appends it 
> to the agent log.
> 
> This is to facilitate debugging spurious fetch failures in production or CI.
> 
> Similar, but not the same: https://reviews.apache.org/r/37813/ (see 
> MESOS-3743 for an explanation).
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 2b2298c329ed5fb5863cb0fed1491e478c3e5d5a 
> 
> Diff: https://reviews.apache.org/r/39338/diff/
> 
> 
> Testing
> ---
> 
> Ran make check. As expected no change in behavior.
> When I modified the fetcher to fail, 
> I observed the expected extra output.
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 39338: Added code that appends the fetcher log to the agent log upon fetcher failure.

2015-10-16 Thread Adam B 


> On Oct. 15, 2015, 6:24 a.m., Benjamin Bannier wrote:
> > src/slave/containerizer/fetcher.cpp, line 799
> > 
> >
> > It would probably be better to stream the full message into the `LOG` 
> > object to get the full message into a single block in the block in face of 
> > concurrent `LOG` calls from other threads.

FetcherProcess is a single-threaded actor, so it shouldn't have other threads 
itself. But it's logging to the same log as the SlaveProcess, and 
StatusUpdateManager, so those could interleave. And do we run multiple 
FetcherProcesses? It'd be extra confusing to not know which fetcher log went to 
which task if two tasks failed simultaneously.
We should probably also log the containerId and fetcher command, so we can 
differentiate between different failures and see the failed command without 
enabling VLOG.
```
LOG(WARNING) << "Begin " << containerId << " fetcher log (stderr in sandbox) 
from running: "
 << command << "\n"
 << text.get() << "\n"
 << "End " << containerId << " fetcher log";
```


- Adam


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


On Oct. 15, 2015, 6:11 a.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39338/
> ---
> 
> (Updated Oct. 15, 2015, 6:11 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Ben Mahler, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3743
> https://issues.apache.org/jira/browse/MESOS-3743
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added an onFailed() clause to the inspection of the fetcher subprocess run. 
> This clause copies the fetcher log from /stderr and appends it 
> to the agent log.
> 
> This is to facilitate debugging spurious fetch failures in production or CI.
> 
> Similar, but not the same: https://reviews.apache.org/r/37813/ (see 
> MESOS-3743 for an explanation).
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 2b2298c329ed5fb5863cb0fed1491e478c3e5d5a 
> 
> Diff: https://reviews.apache.org/r/39338/diff/
> 
> 
> Testing
> ---
> 
> Ran make check. As expected no change in behavior.
> When I modified the fetcher to fail, 
> I observed the expected extra output.
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 39338: Added code that appends the fetcher log to the agent log upon fetcher failure.

2015-10-16 Thread Bernd Mathiske 


> On Oct. 16, 2015, 12:29 a.m., Adam B wrote:
> > src/slave/containerizer/fetcher.cpp, line 794
> > 
> >
> > What's this string parameter that you're ignoring? If it's the Failure 
> > message, I'd think you'd want to log that too.
> > 
> > Should this lambda return a Future too? Otherwise, how will it 
> > run the following onAny cleanup below?
> 
> Bernd Mathiske wrote:
> onFailed() takes a parameter of this type:
> 
>   typedef lambda::function FailedCallback;
>   
> The string parameter of this callback receives the error message from the 
> failed future as argument when onFailed is called. The latter is declared 
> this way:
> 
>   const Future& onFailed(FailedCallback&& callback) const;
> 
> The future you refer to is passed through every onFailed call itself, not 
> through its callback, which must return void.

The failure message has already been logged at this point.


- Bernd


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


On Oct. 16, 2015, 1:56 a.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39338/
> ---
> 
> (Updated Oct. 16, 2015, 1:56 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-3743
> https://issues.apache.org/jira/browse/MESOS-3743
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added an onFailed() clause to the inspection of the fetcher subprocess run. 
> This clause copies the fetcher log from /stderr and appends it 
> to the agent log.
> 
> This is to facilitate debugging spurious fetch failures in production or CI.
> 
> Similar, but not the same: https://reviews.apache.org/r/37813/ (see 
> MESOS-3743 for an explanation).
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 2b2298c329ed5fb5863cb0fed1491e478c3e5d5a 
> 
> Diff: https://reviews.apache.org/r/39338/diff/
> 
> 
> Testing
> ---
> 
> Ran make check. As expected no change in behavior.
> When I modified the fetcher to fail, 
> I observed the expected extra output.
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 39338: Added code that appends the fetcher log to the agent log upon fetcher failure.

2015-10-16 Thread Bernd Mathiske 


> On Oct. 16, 2015, 12:29 a.m., Adam B wrote:
> > src/slave/containerizer/fetcher.cpp, line 794
> > 
> >
> > What's this string parameter that you're ignoring? If it's the Failure 
> > message, I'd think you'd want to log that too.
> > 
> > Should this lambda return a Future too? Otherwise, how will it 
> > run the following onAny cleanup below?

onFailed() takes a parameter of this type:

  typedef lambda::function FailedCallback;
  
The string parameter of this callback receives the error message from the 
failed future as argument when onFailed is called. The latter is declared this 
way:

  const Future& onFailed(FailedCallback&& callback) const;

The future you refer to is passed through every onFailed call itself, not 
through its callback, which must return void.


- Bernd


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


On Oct. 15, 2015, 6:11 a.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39338/
> ---
> 
> (Updated Oct. 15, 2015, 6:11 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Ben Mahler, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3743
> https://issues.apache.org/jira/browse/MESOS-3743
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added an onFailed() clause to the inspection of the fetcher subprocess run. 
> This clause copies the fetcher log from /stderr and appends it 
> to the agent log.
> 
> This is to facilitate debugging spurious fetch failures in production or CI.
> 
> Similar, but not the same: https://reviews.apache.org/r/37813/ (see 
> MESOS-3743 for an explanation).
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 2b2298c329ed5fb5863cb0fed1491e478c3e5d5a 
> 
> Diff: https://reviews.apache.org/r/39338/diff/
> 
> 
> Testing
> ---
> 
> Ran make check. As expected no change in behavior.
> When I modified the fetcher to fail, 
> I observed the expected extra output.
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 39338: Added code that appends the fetcher log to the agent log upon fetcher failure.

2015-10-16 Thread Bernd Mathiske 

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

(Updated Oct. 16, 2015, 1:56 a.m.)


Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy 
Chen.


Changes
---

Followed @bbannier's advice.


Bugs: MESOS-3743
https://issues.apache.org/jira/browse/MESOS-3743


Repository: mesos


Description
---

Added an onFailed() clause to the inspection of the fetcher subprocess run. 
This clause copies the fetcher log from /stderr and appends it to 
the agent log.

This is to facilitate debugging spurious fetch failures in production or CI.

Similar, but not the same: https://reviews.apache.org/r/37813/ (see MESOS-3743 
for an explanation).


Diffs (updated)
-

  src/slave/containerizer/fetcher.cpp 2b2298c329ed5fb5863cb0fed1491e478c3e5d5a 

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


Testing
---

Ran make check. As expected no change in behavior.
When I modified the fetcher to fail, 
I observed the expected extra output.


Thanks,

Bernd Mathiske

Re: Review Request 39338: Added code that appends the fetcher log to the agent log upon fetcher failure.

2015-10-16 Thread Adam B 


> On Oct. 16, 2015, 12:29 a.m., Adam B wrote:
> > src/slave/containerizer/fetcher.cpp, line 794
> > 
> >
> > What's this string parameter that you're ignoring? If it's the Failure 
> > message, I'd think you'd want to log that too.
> > 
> > Should this lambda return a Future too? Otherwise, how will it 
> > run the following onAny cleanup below?
> 
> Bernd Mathiske wrote:
> onFailed() takes a parameter of this type:
> 
>   typedef lambda::function FailedCallback;
>   
> The string parameter of this callback receives the error message from the 
> failed future as argument when onFailed is called. The latter is declared 
> this way:
> 
>   const Future& onFailed(FailedCallback&& callback) const;
> 
> The future you refer to is passed through every onFailed call itself, not 
> through its callback, which must return void.
> 
> Bernd Mathiske wrote:
> The failure message has already been logged at this point.

Ok, that makes more sense to me. Just trying to figure out what happens to the 
Failure message if you're ignoring it in onFailed(). So onFailed() 
automatically passes on the (failed) Future to the onAny() below, and after 
that it becomes the final object in the `return fetcherSubprocess...` statement 
on line 779. Then whatever called FetcherProcess::Run() waits on that future 
and hopefully logs the Failure message somewhere? So will you see the "Begin 
fetcher log..." before "Failed to fetch all URIs for container..."?


- Adam


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


On Oct. 16, 2015, 1:56 a.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39338/
> ---
> 
> (Updated Oct. 16, 2015, 1:56 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-3743
> https://issues.apache.org/jira/browse/MESOS-3743
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added an onFailed() clause to the inspection of the fetcher subprocess run. 
> This clause copies the fetcher log from /stderr and appends it 
> to the agent log.
> 
> This is to facilitate debugging spurious fetch failures in production or CI.
> 
> Similar, but not the same: https://reviews.apache.org/r/37813/ (see 
> MESOS-3743 for an explanation).
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 2b2298c329ed5fb5863cb0fed1491e478c3e5d5a 
> 
> Diff: https://reviews.apache.org/r/39338/diff/
> 
> 
> Testing
> ---
> 
> Ran make check. As expected no change in behavior.
> When I modified the fetcher to fail, 
> I observed the expected extra output.
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Review Request 39338: Added code that appends the fetcher log to the agent log upon fetcher failure.

2015-10-15 Thread Bernd Mathiske 

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

Review request for mesos, Benjamin Bannier, Ben Mahler, and Till Toenshoff.


Bugs: MESOS-3743
https://issues.apache.org/jira/browse/MESOS-3743


Repository: mesos


Description
---

Added an onFailed() clause to the inspection of the fetcher subprocess run. 
This clause copies the fetcher log from /stderr and appends it to 
the agent log.

This is to facilitate debugging spurious fetch failures in production or CI.

Similar, but not the same: https://reviews.apache.org/r/37813/ (see MESOS-3743 
for an explanation).


Diffs
-

  src/slave/containerizer/fetcher.cpp 2b2298c329ed5fb5863cb0fed1491e478c3e5d5a 

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


Testing
---

Ran make check. As expected no change in behavior.
When I modified the fetcher to fail, 
I observed the expected extra output.


Thanks,

Bernd Mathiske

Re: Review Request 39338: Added code that appends the fetcher log to the agent log upon fetcher failure.

2015-10-15 Thread Benjamin Bannier 

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



src/slave/containerizer/fetcher.cpp (line 703)


Could as well be `const`.



src/slave/containerizer/fetcher.cpp (line 799)


It would probably be better to stream the full message into the `LOG` 
object to get the full message into a single block in the block in face of 
concurrent `LOG` calls from other threads.


- Benjamin Bannier


On Oct. 15, 2015, 1:11 p.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39338/
> ---
> 
> (Updated Oct. 15, 2015, 1:11 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Ben Mahler, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3743
> https://issues.apache.org/jira/browse/MESOS-3743
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added an onFailed() clause to the inspection of the fetcher subprocess run. 
> This clause copies the fetcher log from /stderr and appends it 
> to the agent log.
> 
> This is to facilitate debugging spurious fetch failures in production or CI.
> 
> Similar, but not the same: https://reviews.apache.org/r/37813/ (see 
> MESOS-3743 for an explanation).
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 2b2298c329ed5fb5863cb0fed1491e478c3e5d5a 
> 
> Diff: https://reviews.apache.org/r/39338/diff/
> 
> 
> Testing
> ---
> 
> Ran make check. As expected no change in behavior.
> When I modified the fetcher to fail, 
> I observed the expected extra output.
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 39338: Added code that appends the fetcher log to the agent log upon fetcher failure.

2015-10-15 Thread Till Toenshoff 

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

Ship it!



src/slave/containerizer/fetcher.cpp (line 703)


It could but we don't enforce that - actually some of us go as far as 
saying we shouldn't make this const unless we do that for all possibly const 
types (consistency > strictness).


- Till Toenshoff


On Oct. 15, 2015, 1:11 p.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39338/
> ---
> 
> (Updated Oct. 15, 2015, 1:11 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Ben Mahler, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3743
> https://issues.apache.org/jira/browse/MESOS-3743
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added an onFailed() clause to the inspection of the fetcher subprocess run. 
> This clause copies the fetcher log from /stderr and appends it 
> to the agent log.
> 
> This is to facilitate debugging spurious fetch failures in production or CI.
> 
> Similar, but not the same: https://reviews.apache.org/r/37813/ (see 
> MESOS-3743 for an explanation).
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 2b2298c329ed5fb5863cb0fed1491e478c3e5d5a 
> 
> Diff: https://reviews.apache.org/r/39338/diff/
> 
> 
> Testing
> ---
> 
> Ran make check. As expected no change in behavior.
> When I modified the fetcher to fail, 
> I observed the expected extra output.
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 39338: Added code that appends the fetcher log to the agent log upon fetcher failure.

2015-10-15 Thread Marco Massenzio 

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

Ship it!


Ship It!

- Marco Massenzio


On Oct. 15, 2015, 1:11 p.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39338/
> ---
> 
> (Updated Oct. 15, 2015, 1:11 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Ben Mahler, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3743
> https://issues.apache.org/jira/browse/MESOS-3743
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added an onFailed() clause to the inspection of the fetcher subprocess run. 
> This clause copies the fetcher log from /stderr and appends it 
> to the agent log.
> 
> This is to facilitate debugging spurious fetch failures in production or CI.
> 
> Similar, but not the same: https://reviews.apache.org/r/37813/ (see 
> MESOS-3743 for an explanation).
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 2b2298c329ed5fb5863cb0fed1491e478c3e5d5a 
> 
> Diff: https://reviews.apache.org/r/39338/diff/
> 
> 
> Testing
> ---
> 
> Ran make check. As expected no change in behavior.
> When I modified the fetcher to fail, 
> I observed the expected extra output.
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>