On Thu, Apr 25, 2013 at 2:17 PM, Benjamin Hindman < [email protected]> wrote:
> > 1. proc.hpp includes os.hpp, so making os.hpp include proc.hpp >> introduces a circular dependency, no? >> > > I think you might be able to get away with a declaration for os::ls in > proc.hpp rather than having to include all of os.hpp. > Doesn't this imply that the callers must ensure os.hpp is included so that the definition is available? > If not, we had talked about separate files for os functions anyway. I > could imagine an os directory in stout with an ls.hpp, find.hpp, etc, and a > single stout/os.hpp that includes all of those. Then proc.hpp could just > include stout/os/ls.hpp. > > > >> 2. As you mentioned, ProcessStatus is linux specific for the most part. >> We'll either need to make it generic, or #ifdef it accordingly. What did >> you mean by implement things like children differently? >> > > Well, implemented not assuming proc::status is available. ;) > > > > >> >> >> On Thu, Apr 25, 2013 at 1:53 PM, Benjamin Hindman < >> [email protected]> wrote: >> >>> I think we want "everything" (pids, children, alive, etc.) in os:: with >>> implementations that use the proc:: functions for Linux and do other things >>> for Mac OS X. As in, os::pids can be implemented for Linux as a call to >>> proc::pids, while the implementation for Mac OS X can be whatever else it >>> needs to be. The trick here is that ProcessStatus is a pretty specific >>> Linux abstraction, so either we need to make a generic one or we need to >>> implement things like children differently. >>> >>> >>> On Thu, Apr 25, 2013 at 1:41 PM, Benjamin Mahler < >>> [email protected]> wrote: >>> >>>> Ok so linux/proc.hpp currently has: >>>> >>>> Try<std::set<pid_t> > pids(); >>>> Try<std::set<pid_t> > children(pid_t pid, bool recursive = true); >>>> Try<ProcessStatus> status(pid_t pid); >>>> >>>> We want these to work for OSX as well for now. >>>> >>>> Say we move linux/proc.{cpp,hpp} into stout/proc.hpp, then we can move >>>> alive into os and fail the compilation if anyone includes stout/proc.hpp >>>> without __linux__ defined. >>>> >>>> We'll also want os::pids(), os::children(pid_t) and os::status(pid_t) >>>> for non-linux systems. I originally wanted to have these call into >>>> stout/proc.hpp for linux, but that introduces a circular dependency. >>>> >>>> >>>> On Thu, Apr 25, 2013 at 11:51 AM, Benjamin Hindman < >>>> [email protected]> wrote: >>>> >>>>> I like os:: a lot. I think the type signature (i.e., taking a pid_t) >>>>> is sufficient for disambiguation. >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> On Apr 25, 2013, at 11:43 AM, Benjamin Mahler < >>>>> [email protected]> wrote: >>>>> >>>>> We also have a src/common/process_utils.hpp which contains only >>>>> mesos::internal::utils::process::killtree() at the moment. >>>>> >>>>> >>>>> On Thu, Apr 25, 2013 at 11:37 AM, Yan Xu <[email protected]> wrote: >>>>> >>>>>> I guess os:: is fine, but in a separate file? >>>>>> >>>>>> -- >>>>>> Jiang Yan Xu <[email protected]> @xujyan <http://twitter.com/xujyan> >>>>>> >>>>>> >>>>>> On Thu, Apr 25, 2013 at 11:29 AM, Vinod Kone <[email protected]>wrote: >>>>>> >>>>>>> I don't like process:: because it conflicts with the libprocess >>>>>>> namespace as you mentioned. >>>>>>> >>>>>>> I still like proc:: but clearly BenH doesn't like it. I'm ok with >>>>>>> os:: namespace. >>>>>>> >>>>>>> >>>>>>> @vinodkone >>>>>>> >>>>>>> >>>>>>> On Thu, Apr 25, 2013 at 11:19 AM, Benjamin Mahler < >>>>>>> [email protected]> wrote: >>>>>>> >>>>>>>> Is there any consensus on how to place process utilities in stout? >>>>>>>> I would expect this to be in a process:: namespace but of course that >>>>>>>> is >>>>>>>> confusing because we use libprocess, which should perhaps have a >>>>>>>> libprocess:: namespace instead.. >>>>>>>> >>>>>>>> I'll be moving process utilities etc into stout, hopefully with the >>>>>>>> same calls for linux and OSX but I'm not yet certain if that is >>>>>>>> possible. I >>>>>>>> would like to place these in a process.hpp file inside a process:: >>>>>>>> namespace. >>>>>>>> >>>>>>>> I think these read very nicely: >>>>>>>> process::alive(pid_t) >>>>>>>> process::children(pid_t) >>>>>>>> process::stat(pid_t) >>>>>>>> >>>>>>>> Thoughts? >>>>>>>> >>>>>>>> >>>>>>>> On Tue, Apr 23, 2013 at 6:29 PM, Yan Xu <[email protected]> wrote: >>>>>>>> >>>>>>>>> This batch of commits changed the reaper to use "Future" as the >>>>>>>>> notification mechanism. >>>>>>>>> >>>>>>>>> Sequence: >>>>>>>>> https://reviews.apache.org/r/10744/ >>>>>>>>> https://reviews.apache.org/r/10745/ >>>>>>>>> https://reviews.apache.org/r/10746/ >>>>>>>>> https://reviews.apache.org/r/10747/ >>>>>>>>> >>>>>>>>> Best, >>>>>>>>> Yan >>>>>>>>> -- >>>>>>>>> Jiang Yan Xu <[email protected]> @xujyan <http://twitter.com/xujyan> >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
