mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Thomas Rampelberg" <tho...@saunter.org>
Subject Re: Review Request 24469: Created pure python package for the CLI.
Date Fri, 08 Aug 2014 03:29:07 GMT


> On Aug. 8, 2014, 2:05 a.m., Brian Wickman wrote:
> > the following are high level comments, mostly around style and maintainability which
are important for community projects with large numbers of committers.  not all recommendations
need to be taken now, but doing so sooner than later will save time in the future.
> > 
> > 1) I recommend integrating flake8 and isort into your test suite (either via nose
or tox) -- this will catch lots of bugs up front and reduce the burden from reviewers of keeping
style consistent.
> > 
> > 2) At a minimum, add 'from __future__ import absolute_import, print_function' to
each file.  This will promote polyglot 2.x + 3.x by throwing SyntaxErrors for statement prints
and prevent ambiguous imports.
> > 
> > 3) There are a lot of places where you use itertools.i* and map/filter etc.  This
style of code, while written with the best of intentions re: performance, may be premature
optimization and introduces subtle bugs that make it hard to transition to 3.x.  I've tried
to point out a bunch of these cases but will have certainly missed some.
> > 
> > 4) I recommend against using gevent for mesos.cli -- afaict it is the only requirement
here that is not pure python, and it's only used in a couple places where subprocess + ThreadPool
would be sufficient.  Only recommending this from my experience maintaining egg/wheel dependencies
across a large engineering organization.  C extensions = pain for tools intended to run on
dev laptops, less so for applications in a homogenous production environment.
> >

My goodness. Thanks for taking the time, all great feedback! I'll get it updated =)


- Thomas


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


On Aug. 8, 2014, 12:37 a.m., Thomas Rampelberg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24469/
> -----------------------------------------------------------
> 
> (Updated Aug. 8, 2014, 12:37 a.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Bugs: mesos-1016
>     https://issues.apache.org/jira/browse/mesos-1016
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> This is a re-implementation of the CLI tools that removes the dependencies on compiled
code and implements everything purely in python. You will now be able to `pip install mesos.cli`
and get these tools anywhere (such as developer's laptops who don't have mesos itself installed
or even a windows machine).
> 
> The interface has changed and the tool has been made task centric. You can configure
the master you'd like to use and then ignore which framework a task is running under completely.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 39af0365e429b8d08addadb09ee18080a19625f8 
>   src/cli/mesos-cat 73dc63ebc2fab9150f4dd691e10defaf989b9e6b 
>   src/cli/mesos-ps ddd9ec5dd0045d168ee4ed840194fe18c304b56a 
>   src/cli/mesos-scp 77b8557d8ca33960d9135ad4fa6bfe3dcd087108 
>   src/cli/mesos-tail 256a804b98b2efb2fb0256635449b36a3a4d0a6b 
>   src/cli/mesos.cpp 171a707cd2ba2348898e7fbe8fe9f0634edd6d86 
>   src/cli/python/mesos/__init__.py 028b0d27fb193bac96f2a6a3201ee4cc8fd369ef 
>   src/cli/python/mesos/cli.py 857059e2c12ed7f1419dfbf0d11dda0ff9fae235 
>   src/cli/python/mesos/futures.py da2f4ceb72f4d8f1a1a48b0c31111a1723b2f638 
>   src/cli/python/mesos/http.py 0e19aa8dd6595a9b292189364fd51fb9b3bfb285 
>   src/cli/resolve.cpp a99b6094dffc9f7aa44bcf63ad40121e1abb120b 
>   src/python/cli/README.rst PRE-CREATION 
>   src/python/cli/bin/mesos-zsh-completion.sh PRE-CREATION 
>   src/python/cli/docs/debugging.md PRE-CREATION 
>   src/python/cli/mesos/__init__.py PRE-CREATION 
>   src/python/cli/mesos/cli/__init__.py PRE-CREATION 
>   src/python/cli/mesos/cli/cfg.py PRE-CREATION 
>   src/python/cli/mesos/cli/cli.py PRE-CREATION 
>   src/python/cli/mesos/cli/cmds/cat.py PRE-CREATION 
>   src/python/cli/mesos/cli/cmds/completion.py PRE-CREATION 
>   src/python/cli/mesos/cli/cmds/config.py PRE-CREATION 
>   src/python/cli/mesos/cli/cmds/events.py PRE-CREATION 
>   src/python/cli/mesos/cli/cmds/find.py PRE-CREATION 
>   src/python/cli/mesos/cli/cmds/head.py PRE-CREATION 
>   src/python/cli/mesos/cli/cmds/help.py PRE-CREATION 
>   src/python/cli/mesos/cli/cmds/ls.py PRE-CREATION 
>   src/python/cli/mesos/cli/cmds/ps.py PRE-CREATION 
>   src/python/cli/mesos/cli/cmds/resolve.py PRE-CREATION 
>   src/python/cli/mesos/cli/cmds/scp.py PRE-CREATION 
>   src/python/cli/mesos/cli/cmds/ssh.py PRE-CREATION 
>   src/python/cli/mesos/cli/cmds/state.py PRE-CREATION 
>   src/python/cli/mesos/cli/cmds/tail.py PRE-CREATION 
>   src/python/cli/mesos/cli/exceptions.py PRE-CREATION 
>   src/python/cli/mesos/cli/log.py PRE-CREATION 
>   src/python/cli/mesos/cli/main.py PRE-CREATION 
>   src/python/cli/mesos/cli/master.py PRE-CREATION 
>   src/python/cli/mesos/cli/mesos_file.py PRE-CREATION 
>   src/python/cli/mesos/cli/slave.py PRE-CREATION 
>   src/python/cli/mesos/cli/ssh.py PRE-CREATION 
>   src/python/cli/mesos/cli/state.py PRE-CREATION 
>   src/python/cli/mesos/cli/task.py PRE-CREATION 
>   src/python/cli/mesos/cli/util.py PRE-CREATION 
>   src/python/cli/mesos/cli/zookeeper.py PRE-CREATION 
>   src/python/cli/setup.cfg PRE-CREATION 
>   src/python/cli/setup.py PRE-CREATION 
>   src/python/cli/tests/__init__.py PRE-CREATION 
>   src/python/cli/tests/data/browse.json PRE-CREATION 
>   src/python/cli/tests/data/config.json PRE-CREATION 
>   src/python/cli/tests/data/master-host PRE-CREATION 
>   src/python/cli/tests/data/master.pb PRE-CREATION 
>   src/python/cli/tests/data/master_state.json PRE-CREATION 
>   src/python/cli/tests/data/sandbox/stderr PRE-CREATION 
>   src/python/cli/tests/data/sandbox/stdout PRE-CREATION 
>   src/python/cli/tests/data/slave-20140619-151434-16842879-5050-1196-0.json PRE-CREATION

>   src/python/cli/tests/data/slave_statistics.json PRE-CREATION 
>   src/python/cli/tests/integration/__init__.py PRE-CREATION 
>   src/python/cli/tests/integration/test_cat.py PRE-CREATION 
>   src/python/cli/tests/integration/test_completion.py PRE-CREATION 
>   src/python/cli/tests/integration/test_config.py PRE-CREATION 
>   src/python/cli/tests/integration/test_find.py PRE-CREATION 
>   src/python/cli/tests/integration/test_head.py PRE-CREATION 
>   src/python/cli/tests/integration/test_ls.py PRE-CREATION 
>   src/python/cli/tests/integration/test_ps.py PRE-CREATION 
>   src/python/cli/tests/integration/test_resolve.py PRE-CREATION 
>   src/python/cli/tests/integration/test_scp.py PRE-CREATION 
>   src/python/cli/tests/integration/test_ssh.py PRE-CREATION 
>   src/python/cli/tests/integration/test_state.py PRE-CREATION 
>   src/python/cli/tests/integration/test_tail.py PRE-CREATION 
>   src/python/cli/tests/utils.py PRE-CREATION 
>   src/python/cli/tox.ini PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/24469/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Thomas Rampelberg
> 
>


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