mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Joseph Wu" <jos...@mesosphere.io>
Subject Re: Review Request 42059: Updated ContainerLogger to use Subprocess::IO type.
Date Wed, 13 Jan 2016 22:02:55 GMT


> On Jan. 13, 2016, 11:11 a.m., Benjamin Hindman wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, lines 829-832
> > <https://reviews.apache.org/r/42059/diff/1/?file=1187114#file1187114line829>
> >
> >     Subprocess::IO out = Subprocess::FD(STDOUT_FILENO);
> >     Subprocess::IO err = Subprocess::FD(STDERR_FILENO);
> >     
> >     if (!local) {
> >       out = subprocessInfo.out;
> >       err = subprocessInfo.err;
> >     }

I remember why I didn't do this before.  Essentially, you would need to:

1) Change the field `process::Owned<process::Subprocess::IO> io;` to a public field.
 (Making this public also breaks the ContainerLogger's restrictions on the types of `Subprocess::IO`.)
2) Add this to the containerizer:
```
    Owned<Subprocess::IO> out(new Subprocess::FD(STDOUT_FILENO));
    Owned<Subprocess::IO> err(new Subprocess::FD(STDERR_FILENO));
    if (!local) {
      out.reset(subprocessInfo.out.io.release());
      err.reset(subprocessInfo.err.io.release());
    }
```

---

Addressing your original point (that `SubprocessInfo` non-obviously redirects to STDOUT_FILENO
and STDERR_FILENO by default), I think it would be neater to add a NOTE here instead.

---

Note: Trying to use a flavor of the ternary, i.e.
```
(local ? Subprocess::FD(STDOUT_FILENO) : subprocessInfo.out)
```

Gives the compiler error:
"error: allocating an object of abstract class type 'Subprocess::IO'"


- Joseph


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


On Jan. 13, 2016, 2:02 p.m., Joseph Wu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42059/
> -----------------------------------------------------------
> 
> (Updated Jan. 13, 2016, 2:02 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Artem Harutyunyan.
> 
> 
> Bugs: MESOS-4136
>     https://issues.apache.org/jira/browse/MESOS-4136
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Update ContainerLogger to use Subprocess::IO type
> 
> 
> Diffs
> -----
> 
>   include/mesos/slave/container_logger.hpp a2362070ead0afcef1e6c2ca784083ddb01ba51a 
>   src/slave/containerizer/mesos/containerizer.cpp f3c370aeb331beb6202fd30cd0278877da0b42e0

> 
> Diff: https://reviews.apache.org/r/42059/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>


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