----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15260/#review28286 -----------------------------------------------------------
Ship it! Cool, seems like we may want to use that python Future wrapper for the v1 Python API, I made some comments on it hoping to beef it up for that reason. Wrapping Future is great but our code does not work with Python 3.x (most notably the way we use print is not compatible). A good guide for supporting both is here: http://python3porting.com/noconv.html Hopefully one day we can have CI on both Python 2.x and 3.x to ensure we keep support. bin/mesos.sh.in <https://reviews.apache.org/r/15260/#comment55082> Does this need a similar todo for PIP / setup.py or will this remain? src/cli/mesos-ps <https://reviews.apache.org/r/15260/#comment55085> Something like: # Command executors share the same ID as the task, but this is not communicated by the master. src/cli/mesos-ps <https://reviews.apache.org/r/15260/#comment55086> 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): src/cli/mesos-ps <https://reviews.apache.org/r/15260/#comment55088> Ditto for the command executor comment. src/cli/mesos-ps <https://reviews.apache.org/r/15260/#comment55089> Consider using parens instead of backlash as above. src/cli/mesos-ps <https://reviews.apache.org/r/15260/#comment55090> 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 ;) src/cli/mesos-ps <https://reviews.apache.org/r/15260/#comment55091> neat! src/cli/mesos.cpp <https://reviews.apache.org/r/15260/#comment55081> Does this need a similar todo for PIP / setup.py or will this remain? src/cli/python/mesos/cli.py <https://reviews.apache.org/r/15260/#comment55080> This comment could be more clear on what is returned and what the 'master' argument is supposed to be. src/cli/python/mesos/futures.py <https://reviews.apache.org/r/15260/#comment55077> 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 src/cli/python/mesos/futures.py <https://reviews.apache.org/r/15260/#comment55069> Consider: def running(self): return not done() src/cli/python/mesos/futures.py <https://reviews.apache.org/r/15260/#comment55067> 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! src/cli/python/mesos/futures.py <https://reviews.apache.org/r/15260/#comment55068> We should either handle the wait=False case or guard against it with: raise NotImplementedError Of course, preferably we can implement wait=False. src/cli/python/mesos/futures.py <https://reviews.apache.org/r/15260/#comment55065> s/inorder/in order/ src/cli/python/mesos/futures.py <https://reviews.apache.org/r/15260/#comment55066> How about: return max(remaining, 0) - Ben Mahler 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 > >
