> On Nov. 6, 2013, 7:48 p.m., Ben Mahler wrote: > > bin/mesos.sh.in, lines 30-33 > > <https://reviews.apache.org/r/15260/diff/1/?file=378892#file378892line30> > > > > Does this need a similar todo for PIP / setup.py or will this remain?
Added a TODO. > On Nov. 6, 2013, 7:48 p.m., Ben Mahler wrote: > > src/cli/mesos-ps, line 167 > > <https://reviews.apache.org/r/15260/diff/1/?file=378894#file378894line167> > > > > Something like: > > > > # Command executors share the same ID as the task, but this is not > > communicated by the master. Yes, in the shuffle I forgot to add this very handy comment! Thanks! > On Nov. 6, 2013, 7:48 p.m., Ben Mahler wrote: > > src/cli/mesos-ps, lines 171-172 > > <https://reviews.apache.org/r/15260/diff/1/?file=378894#file378894line171> > > > > The idiom is to use parens instead of the backslash for line > > continuations I believe: > > > > if (entry['framework_id'] == framework_id and > > entry['executor_id'] == executor_id): Awesome, thanks! > On Nov. 6, 2013, 7:48 p.m., Ben Mahler wrote: > > src/cli/mesos-ps, lines 194-195 > > <https://reviews.apache.org/r/15260/diff/1/?file=378894#file378894line194> > > > > Consider using parens instead of backlash as above. Yes, thanks! > On Nov. 6, 2013, 7:48 p.m., Ben Mahler wrote: > > src/cli/mesos-ps, line 217 > > <https://reviews.apache.org/r/15260/diff/1/?file=378894#file378894line217> > > > > We can support Python 3 by using print as a function, feel free to punt > > but at some point we should take this up! > > > > Maybe someone else will have sufficient motivation ;) I took it on but just used sys.std* everywhere. Thanks! > On Nov. 6, 2013, 7:48 p.m., Ben Mahler wrote: > > src/cli/mesos-ps, line 229 > > <https://reviews.apache.org/r/15260/diff/1/?file=378894#file378894line229> > > > > neat! Indeed. > On Nov. 6, 2013, 7:48 p.m., Ben Mahler wrote: > > src/cli/mesos.cpp, lines 71-73 > > <https://reviews.apache.org/r/15260/diff/1/?file=378895#file378895line71> > > > > Does this need a similar todo for PIP / setup.py or will this remain? Added a TODO. > On Nov. 6, 2013, 7:48 p.m., Ben Mahler wrote: > > src/cli/python/mesos/futures.py, lines 29-30 > > <https://reviews.apache.org/r/15260/diff/1/?file=378897#file378897line29> > > > > Consider: > > > > def running(self): > > return not done() I'm assuming you meant 'return not self.done()'. > On Nov. 6, 2013, 7:48 p.m., Ben Mahler wrote: > > src/cli/python/mesos/futures.py, line 23 > > <https://reviews.apache.org/r/15260/diff/1/?file=378897#file378897line23> > > > > False works but seems to provide the wrong semantics since the call is > > not necessarily in progress, consider: > > > > def cancel(self): > > # TODO(benh): Implement cancellation. > > raise NotImplementedError I'm going to keep it as 'False' for now because a lot of the futures that get created can't be cancelled since they're backed by threads. I agree that we could add in an extra check to see if the future has actually started but since there isn't much precedent for that in the API yet I've just added a TODO. > On Nov. 6, 2013, 7:48 p.m., Ben Mahler wrote: > > src/cli/python/mesos/futures.py, line 97 > > <https://reviews.apache.org/r/15260/diff/1/?file=378897#file378897line97> > > > > Is this based on ThreadPoolExecutor? > > > > If so, it seems like Python 3.x code will not work when we successfully > > import from concurrent.futures since ThreadingExecutor is not defined there? > > > > We should add the missing map() function too! Yes, since this a fresh implementation of an executor that actually creates a thread per submit. I updated the code so that the ThreadingExecutor was defined irregardless of Python 3.x or not. I also added a map() that raises NotImplementedError. Will get to that one later! > On Nov. 6, 2013, 7:48 p.m., Ben Mahler wrote: > > src/cli/python/mesos/futures.py, line 114 > > <https://reviews.apache.org/r/15260/diff/1/?file=378897#file378897line114> > > > > We should either handle the wait=False case or guard against it with: > > raise NotImplementedError > > > > Of course, preferably we can implement wait=False. I wrapped the code with an 'if wait:', i.e., I only join the threads if wait=True. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15260/#review28286 ----------------------------------------------------------- On Nov. 6, 2013, 9:01 a.m., Benjamin Hindman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/15260/ > ----------------------------------------------------------- > > (Updated Nov. 6, 2013, 9:01 a.m.) > > > Review request for mesos, Ben Mahler, Du Li, Shingo Omura, Niklas Nielsen, > and Vinod Kone. > > > Repository: mesos-git > > > Description > ------- > > The initial Python CLI 'mesos-ps' tool used curl in order to make > concurrent HTTP requests. This was a giant hack (one which even > required getting the maximum open file descriptor limit to not > fork/exec too many curl subprocesses). Instead of using curl we know > use futures and threads. The choice to use futures was (a) because it > naturally abstracts what we're trying to do and (b) it's "pro style" > for Python > 3.x. For now, we've implemented the futures interface > minimally to accomplish what we need (see > src/cli/python/mesos/futures.py). In addition, we updated the way > mesos-ps was implemented to leverage the futures API. > > To enable sharing of CLI utilities written in Python we've also added > a mechanism for installing Python modules. This will likely change in > the future to use PIP but for now we manually update PYTHONPATH (see > bin/mesos.sh.in and src/cli/mesos.cpp). > > Finally, all of the CLI specific Python code has been updated to use > the standard four space indent rather than two. > > > Diffs > ----- > > bin/mesos.sh.in 3d03ee4a02d90865fbc6fcfec2d3e44071bdcc1f > src/Makefile.am 9780d07a23ca196c541a44a85499c2f44a574b9c > src/cli/mesos-ps b7dca14977fb6cd28ca5f8ef65f2714d342b5c8d > src/cli/mesos.cpp 17a9f0c1553a0e1db060b3e3dd5788ff5cc759fe > src/cli/python/mesos/cli.py PRE-CREATION > src/cli/python/mesos/futures.py PRE-CREATION > > Diff: https://reviews.apache.org/r/15260/diff/ > > > Testing > ------- > > $ make check > $ ./bin/mesos.sh ps --master=... > $ make install > $ mesos ps --master=... > > > Thanks, > > Benjamin Hindman > >
