You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Eric Chung <ci...@gmail.com> on 2018/01/01 01:52:44 UTC

Re: Review Request 61172: Added mesos.http and mesos.exceptions for CLI.

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

(Updated Jan. 1, 2018, 1:52 a.m.)


Review request for mesos, Armand Grillet, Jason Lai, and Kevin Klues.


Repository: mesos


Description (updated)
-------

Part of MESOS-7310, this patch adds the mesos.http and mesos.exceptions
modules, which provides a Resource class and its descendants for
abstracting away common operations over http connectioins with JSON
serialization.

updated patch according to klueska's recommendations


addressed comments


Diffs (updated)
-----

  src/python/cli_new/bootstrap c84e549e59bc7a07b5b70c24c63bc0c16881e442 
  src/python/lib/mesos/exceptions.py PRE-CREATION 
  src/python/lib/mesos/http.py PRE-CREATION 
  src/python/lib/requirements.in e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 
  src/python/lib/tests/conftest.py PRE-CREATION 
  src/python/lib/tests/test_exceptions.py PRE-CREATION 
  src/python/lib/tests/test_http.py PRE-CREATION 
  src/python/lib/tox.ini 8ad030d2dbf1fb7a04f1eaadb587462b4ef3f054 
  support/pylint.config f74f553e238553bd6a6c06f4dd888cc5954a33eb 


Diff: https://reviews.apache.org/r/61172/diff/8/

Changes: https://reviews.apache.org/r/61172/diff/7-8/


Testing
-------

install tox
cd src/python/lib
tox


Thanks,

Eric Chung

Re: Review Request 61172: Added mesos.http and mesos.exceptions for CLI.

Posted by Eric Chung <ci...@gmail.com>.

> On Jan. 18, 2018, 1:59 p.m., Kevin Klues wrote:
> > src/python/lib/mesos/exceptions.py
> > Lines 27-33 (patched)
> > <https://reviews.apache.org/r/61172/diff/7-9/?file=1904480#file1904480line27>
> >
> >     I would prefer to do this at each call site instead of wrapping it up and hiding it in here. This will keep it consistent with the rest of the code base.
> >     
> >     That said, I do like keeping track of the original exception. So something like the following would be better in my opinion:
> >     
> >     ```
> >         def __init__(self, message=None, original_exception=None):
> >             self.original_exception = original_exception
> >             super(MesosException, self).__init__(message)
> >     ```
> >     
> >     Is the name `original_exception` the best name here? Is there a more standard way of wrapping exceptions like this in python and keeping track of the original ones?

it seems there is no good solution for this use case in python 2, however it is supported natively in python 3 via exception chaining: https://www.python.org/dev/peps/pep-3134/ maybe another incentive for us to migrate. :)

just curious though, are you saying i shouldn't append the exception to the message?


- Eric


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


On Jan. 19, 2018, 8:48 p.m., Eric Chung wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61172/
> -----------------------------------------------------------
> 
> (Updated Jan. 19, 2018, 8:48 p.m.)
> 
> 
> Review request for mesos, Armand Grillet, Jason Lai, and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added mesos.http and mesos.exceptions for CLI.
> 
> Part of MESOS-7310, this patch adds the mesos.http and mesos.exceptions
> modules, which provides a Resource class and its descendants for
> abstracting away common operations over http connectioins with JSON
> serialization.
> 
> updated patch according to klueska's recommendations
> 
> 
> Diffs
> -----
> 
>   src/python/cli_new/bootstrap c84e549e59bc7a07b5b70c24c63bc0c16881e442 
>   src/python/lib/mesos/exceptions.py PRE-CREATION 
>   src/python/lib/mesos/http.py PRE-CREATION 
>   src/python/lib/requirements.in e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 
>   src/python/lib/tests/conftest.py PRE-CREATION 
>   src/python/lib/tests/test_exceptions.py PRE-CREATION 
>   src/python/lib/tests/test_http.py PRE-CREATION 
>   src/python/lib/tox.ini 8ad030d2dbf1fb7a04f1eaadb587462b4ef3f054 
>   support/pylint.config f74f553e238553bd6a6c06f4dd888cc5954a33eb 
> 
> 
> Diff: https://reviews.apache.org/r/61172/diff/10/
> 
> 
> Testing
> -------
> 
> install tox
> cd src/python/lib
> tox
> 
> 
> Thanks,
> 
> Eric Chung
> 
>

Re: Review Request 61172: Added mesos.http and mesos.exceptions for CLI.

Posted by Kevin Klues <kl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61172/#review195718
-----------------------------------------------------------




src/python/lib/mesos/exceptions.py
Lines 27-33 (patched)
<https://reviews.apache.org/r/61172/#comment275006>

    I would prefer to do this at each call site instead of wrapping it up and hiding it in here. This will keep it consistent with the rest of the code base.
    
    That said, I do like keeping track of the original exception. So something like the following would be better in my opinion:
    
    ```
        def __init__(self, message=None, original_exception=None):
            self.original_exception = original_exception
            super(MesosException, self).__init__(message)
    ```
    
    Is the name `original_exception` the best name here? Is there a more standard way of wrapping exceptions like this in python and keeping track of the original ones?



src/python/lib/mesos/exceptions.py
Line 50 (original), 61 (patched)
<https://reviews.apache.org/r/61172/#comment275007>

    Can you add the text `The url '{url}' ...` to the beginning of the string so that it composes well with other exceptions. They should all begin with a capital letter.



src/python/lib/mesos/http.py
Lines 201-205 (original), 218-221 (patched)
<https://reviews.apache.org/r/61172/#comment275008>

    Can we simplify this to:
    ```
    if know_exception:
        raise known_exception(response)
    else:
        raise MesosHTTPException(response)
    ```



src/python/lib/mesos/http.py
Line 278 (original), 290 (patched)
<https://reviews.apache.org/r/61172/#comment275009>

    Error strings should beign with a capital letter.



src/python/lib/mesos/http.py
Line 322 (original), 333 (patched)
<https://reviews.apache.org/r/61172/#comment275010>

    Error strings should beign with a capital letter.



src/python/lib/tests/test_http.py
Line 283 (original), 286 (patched)
<https://reviews.apache.org/r/61172/#comment275011>

    The tmeout parameter should be moved to the next line


- Kevin Klues


On Jan. 18, 2018, 4:25 a.m., Eric Chung wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61172/
> -----------------------------------------------------------
> 
> (Updated Jan. 18, 2018, 4:25 a.m.)
> 
> 
> Review request for mesos, Armand Grillet, Jason Lai, and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Part of MESOS-7310, this patch adds the mesos.http and mesos.exceptions
> modules, which provides a Resource class and its descendants for
> abstracting away common operations over http connectioins with JSON
> serialization.
> 
> updated patch according to klueska's recommendations
> 
> 
> Diffs
> -----
> 
>   src/python/cli_new/bootstrap c84e549e59bc7a07b5b70c24c63bc0c16881e442 
>   src/python/lib/mesos/exceptions.py PRE-CREATION 
>   src/python/lib/mesos/http.py PRE-CREATION 
>   src/python/lib/requirements.in e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 
>   src/python/lib/tests/conftest.py PRE-CREATION 
>   src/python/lib/tests/test_exceptions.py PRE-CREATION 
>   src/python/lib/tests/test_http.py PRE-CREATION 
>   src/python/lib/tox.ini 8ad030d2dbf1fb7a04f1eaadb587462b4ef3f054 
>   support/pylint.config f74f553e238553bd6a6c06f4dd888cc5954a33eb 
> 
> 
> Diff: https://reviews.apache.org/r/61172/diff/9/
> 
> 
> Testing
> -------
> 
> install tox
> cd src/python/lib
> tox
> 
> 
> Thanks,
> 
> Eric Chung
> 
>

Re: Review Request 61172: Added mesos.http and mesos.exceptions for CLI.

Posted by Armand Grillet <ag...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61172/#review196638
-----------------------------------------------------------


Ship it!




Ship It!

- Armand Grillet


On Jan. 19, 2018, 8:48 p.m., Eric Chung wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61172/
> -----------------------------------------------------------
> 
> (Updated Jan. 19, 2018, 8:48 p.m.)
> 
> 
> Review request for mesos, Armand Grillet, Jason Lai, and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added mesos.http and mesos.exceptions for CLI.
> 
> Part of MESOS-7310, this patch adds the mesos.http and mesos.exceptions
> modules, which provides a Resource class and its descendants for
> abstracting away common operations over http connectioins with JSON
> serialization.
> 
> updated patch according to klueska's recommendations
> 
> 
> Diffs
> -----
> 
>   src/python/cli_new/bootstrap c84e549e59bc7a07b5b70c24c63bc0c16881e442 
>   src/python/lib/mesos/exceptions.py PRE-CREATION 
>   src/python/lib/mesos/http.py PRE-CREATION 
>   src/python/lib/requirements.in e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 
>   src/python/lib/tests/conftest.py PRE-CREATION 
>   src/python/lib/tests/test_exceptions.py PRE-CREATION 
>   src/python/lib/tests/test_http.py PRE-CREATION 
>   src/python/lib/tox.ini 8ad030d2dbf1fb7a04f1eaadb587462b4ef3f054 
>   support/pylint.config f74f553e238553bd6a6c06f4dd888cc5954a33eb 
> 
> 
> Diff: https://reviews.apache.org/r/61172/diff/10/
> 
> 
> Testing
> -------
> 
> install tox
> cd src/python/lib
> tox
> 
> 
> Thanks,
> 
> Eric Chung
> 
>

Re: Review Request 61172: Added mesos.http and mesos.exceptions for CLI.

Posted by Eric Chung <ci...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61172/
-----------------------------------------------------------

(Updated Feb. 7, 2018, 3:13 p.m.)


Review request for mesos, Armand Grillet, Jason Lai, and Kevin Klues.


Bugs: MESOS-7310
    https://issues.apache.org/jira/browse/MESOS-7310


Repository: mesos


Description
-------

Added mesos.http and mesos.exceptions for CLI.

Part of MESOS-7310, this patch adds the mesos.http and mesos.exceptions
modules, which provides a Resource class and its descendants for
abstracting away common operations over http connectioins with JSON
serialization.

updated patch according to klueska's recommendations


Diffs
-----

  src/python/cli_new/bootstrap c84e549e59bc7a07b5b70c24c63bc0c16881e442 
  src/python/lib/mesos/exceptions.py PRE-CREATION 
  src/python/lib/mesos/http.py PRE-CREATION 
  src/python/lib/requirements.in e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 
  src/python/lib/tests/conftest.py PRE-CREATION 
  src/python/lib/tests/test_exceptions.py PRE-CREATION 
  src/python/lib/tests/test_http.py PRE-CREATION 
  src/python/lib/tox.ini 8ad030d2dbf1fb7a04f1eaadb587462b4ef3f054 
  support/pylint.config f74f553e238553bd6a6c06f4dd888cc5954a33eb 


Diff: https://reviews.apache.org/r/61172/diff/10/


Testing
-------

install tox
cd src/python/lib
tox


Thanks,

Eric Chung

Re: Review Request 61172: Added mesos.http and mesos.exceptions for CLI.

Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61172/#review195852
-----------------------------------------------------------



PASS: Mesos patch 61172 was successfully built and tested.

Reviews applied: `['61172']`

All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/61172

- Mesos Reviewbot Windows


On Jan. 19, 2018, 8:48 p.m., Eric Chung wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61172/
> -----------------------------------------------------------
> 
> (Updated Jan. 19, 2018, 8:48 p.m.)
> 
> 
> Review request for mesos, Armand Grillet, Jason Lai, and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added mesos.http and mesos.exceptions for CLI.
> 
> Part of MESOS-7310, this patch adds the mesos.http and mesos.exceptions
> modules, which provides a Resource class and its descendants for
> abstracting away common operations over http connectioins with JSON
> serialization.
> 
> updated patch according to klueska's recommendations
> 
> 
> Diffs
> -----
> 
>   src/python/cli_new/bootstrap c84e549e59bc7a07b5b70c24c63bc0c16881e442 
>   src/python/lib/mesos/exceptions.py PRE-CREATION 
>   src/python/lib/mesos/http.py PRE-CREATION 
>   src/python/lib/requirements.in e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 
>   src/python/lib/tests/conftest.py PRE-CREATION 
>   src/python/lib/tests/test_exceptions.py PRE-CREATION 
>   src/python/lib/tests/test_http.py PRE-CREATION 
>   src/python/lib/tox.ini 8ad030d2dbf1fb7a04f1eaadb587462b4ef3f054 
>   support/pylint.config f74f553e238553bd6a6c06f4dd888cc5954a33eb 
> 
> 
> Diff: https://reviews.apache.org/r/61172/diff/10/
> 
> 
> Testing
> -------
> 
> install tox
> cd src/python/lib
> tox
> 
> 
> Thanks,
> 
> Eric Chung
> 
>

Re: Review Request 61172: Added mesos.http and mesos.exceptions for CLI.

Posted by Kevin Klues <kl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61172/#review196992
-----------------------------------------------------------


Ship it!




Ship It!

- Kevin Klues


On Jan. 19, 2018, 8:48 p.m., Eric Chung wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61172/
> -----------------------------------------------------------
> 
> (Updated Jan. 19, 2018, 8:48 p.m.)
> 
> 
> Review request for mesos, Armand Grillet, Jason Lai, and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added mesos.http and mesos.exceptions for CLI.
> 
> Part of MESOS-7310, this patch adds the mesos.http and mesos.exceptions
> modules, which provides a Resource class and its descendants for
> abstracting away common operations over http connectioins with JSON
> serialization.
> 
> updated patch according to klueska's recommendations
> 
> 
> Diffs
> -----
> 
>   src/python/cli_new/bootstrap c84e549e59bc7a07b5b70c24c63bc0c16881e442 
>   src/python/lib/mesos/exceptions.py PRE-CREATION 
>   src/python/lib/mesos/http.py PRE-CREATION 
>   src/python/lib/requirements.in e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 
>   src/python/lib/tests/conftest.py PRE-CREATION 
>   src/python/lib/tests/test_exceptions.py PRE-CREATION 
>   src/python/lib/tests/test_http.py PRE-CREATION 
>   src/python/lib/tox.ini 8ad030d2dbf1fb7a04f1eaadb587462b4ef3f054 
>   support/pylint.config f74f553e238553bd6a6c06f4dd888cc5954a33eb 
> 
> 
> Diff: https://reviews.apache.org/r/61172/diff/10/
> 
> 
> Testing
> -------
> 
> install tox
> cd src/python/lib
> tox
> 
> 
> Thanks,
> 
> Eric Chung
> 
>

Re: Review Request 61172: Added mesos.http and mesos.exceptions for CLI.

Posted by Eric Chung <ci...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61172/
-----------------------------------------------------------

(Updated Jan. 19, 2018, 8:48 p.m.)


Review request for mesos, Armand Grillet, Jason Lai, and Kevin Klues.


Repository: mesos


Description (updated)
-------

Added mesos.http and mesos.exceptions for CLI.

Part of MESOS-7310, this patch adds the mesos.http and mesos.exceptions
modules, which provides a Resource class and its descendants for
abstracting away common operations over http connectioins with JSON
serialization.

updated patch according to klueska's recommendations


Diffs (updated)
-----

  src/python/cli_new/bootstrap c84e549e59bc7a07b5b70c24c63bc0c16881e442 
  src/python/lib/mesos/exceptions.py PRE-CREATION 
  src/python/lib/mesos/http.py PRE-CREATION 
  src/python/lib/requirements.in e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 
  src/python/lib/tests/conftest.py PRE-CREATION 
  src/python/lib/tests/test_exceptions.py PRE-CREATION 
  src/python/lib/tests/test_http.py PRE-CREATION 
  src/python/lib/tox.ini 8ad030d2dbf1fb7a04f1eaadb587462b4ef3f054 
  support/pylint.config f74f553e238553bd6a6c06f4dd888cc5954a33eb 


Diff: https://reviews.apache.org/r/61172/diff/10/

Changes: https://reviews.apache.org/r/61172/diff/9-10/


Testing
-------

install tox
cd src/python/lib
tox


Thanks,

Eric Chung

Re: Review Request 61172: Added mesos.http and mesos.exceptions for CLI.

Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61172/#review195690
-----------------------------------------------------------



PASS: Mesos patch 61172 was successfully built and tested.

Reviews applied: `['61172']`

All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/61172

- Mesos Reviewbot Windows


On Jan. 18, 2018, 4:25 a.m., Eric Chung wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61172/
> -----------------------------------------------------------
> 
> (Updated Jan. 18, 2018, 4:25 a.m.)
> 
> 
> Review request for mesos, Armand Grillet, Jason Lai, and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Part of MESOS-7310, this patch adds the mesos.http and mesos.exceptions
> modules, which provides a Resource class and its descendants for
> abstracting away common operations over http connectioins with JSON
> serialization.
> 
> updated patch according to klueska's recommendations
> 
> 
> Diffs
> -----
> 
>   src/python/cli_new/bootstrap c84e549e59bc7a07b5b70c24c63bc0c16881e442 
>   src/python/lib/mesos/exceptions.py PRE-CREATION 
>   src/python/lib/mesos/http.py PRE-CREATION 
>   src/python/lib/requirements.in e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 
>   src/python/lib/tests/conftest.py PRE-CREATION 
>   src/python/lib/tests/test_exceptions.py PRE-CREATION 
>   src/python/lib/tests/test_http.py PRE-CREATION 
>   src/python/lib/tox.ini 8ad030d2dbf1fb7a04f1eaadb587462b4ef3f054 
>   support/pylint.config f74f553e238553bd6a6c06f4dd888cc5954a33eb 
> 
> 
> Diff: https://reviews.apache.org/r/61172/diff/9/
> 
> 
> Testing
> -------
> 
> install tox
> cd src/python/lib
> tox
> 
> 
> Thanks,
> 
> Eric Chung
> 
>

Re: Review Request 61172: Added mesos.http and mesos.exceptions for CLI.

Posted by Eric Chung <ci...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61172/
-----------------------------------------------------------

(Updated Jan. 18, 2018, 4:25 a.m.)


Review request for mesos, Armand Grillet, Jason Lai, and Kevin Klues.


Repository: mesos


Description (updated)
-------

Part of MESOS-7310, this patch adds the mesos.http and mesos.exceptions
modules, which provides a Resource class and its descendants for
abstracting away common operations over http connectioins with JSON
serialization.

updated patch according to klueska's recommendations


Diffs (updated)
-----

  src/python/cli_new/bootstrap c84e549e59bc7a07b5b70c24c63bc0c16881e442 
  src/python/lib/mesos/exceptions.py PRE-CREATION 
  src/python/lib/mesos/http.py PRE-CREATION 
  src/python/lib/requirements.in e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 
  src/python/lib/tests/conftest.py PRE-CREATION 
  src/python/lib/tests/test_exceptions.py PRE-CREATION 
  src/python/lib/tests/test_http.py PRE-CREATION 
  src/python/lib/tox.ini 8ad030d2dbf1fb7a04f1eaadb587462b4ef3f054 
  support/pylint.config f74f553e238553bd6a6c06f4dd888cc5954a33eb 


Diff: https://reviews.apache.org/r/61172/diff/9/

Changes: https://reviews.apache.org/r/61172/diff/8-9/


Testing
-------

install tox
cd src/python/lib
tox


Thanks,

Eric Chung

Re: Review Request 61172: Added mesos.http and mesos.exceptions for CLI.

Posted by Eric Chung <ci...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61172/#review194604
-----------------------------------------------------------




src/python/lib/mesos/http.py
Lines 149 (patched)
<https://reviews.apache.org/r/61172/#comment273467>

    actually this has to be None if we want to allow defaults and overrides. a None passed in indicates that we're not overriding -- if the default value is boolean, there is no way to tell whether this was passed in by the user or whether it was just the default value.


- Eric Chung


On Jan. 1, 2018, 1:52 a.m., Eric Chung wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61172/
> -----------------------------------------------------------
> 
> (Updated Jan. 1, 2018, 1:52 a.m.)
> 
> 
> Review request for mesos, Armand Grillet, Jason Lai, and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Part of MESOS-7310, this patch adds the mesos.http and mesos.exceptions
> modules, which provides a Resource class and its descendants for
> abstracting away common operations over http connectioins with JSON
> serialization.
> 
> updated patch according to klueska's recommendations
> 
> 
> addressed comments
> 
> 
> Diffs
> -----
> 
>   src/python/cli_new/bootstrap c84e549e59bc7a07b5b70c24c63bc0c16881e442 
>   src/python/lib/mesos/exceptions.py PRE-CREATION 
>   src/python/lib/mesos/http.py PRE-CREATION 
>   src/python/lib/requirements.in e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 
>   src/python/lib/tests/conftest.py PRE-CREATION 
>   src/python/lib/tests/test_exceptions.py PRE-CREATION 
>   src/python/lib/tests/test_http.py PRE-CREATION 
>   src/python/lib/tox.ini 8ad030d2dbf1fb7a04f1eaadb587462b4ef3f054 
>   support/pylint.config f74f553e238553bd6a6c06f4dd888cc5954a33eb 
> 
> 
> Diff: https://reviews.apache.org/r/61172/diff/8/
> 
> 
> Testing
> -------
> 
> install tox
> cd src/python/lib
> tox
> 
> 
> Thanks,
> 
> Eric Chung
> 
>

Re: Review Request 61172: Added mesos.http and mesos.exceptions for CLI.

Posted by Eric Chung <ci...@gmail.com>.

> On Jan. 5, 2018, 3:23 p.m., Armand Grillet wrote:
> > src/python/cli_new/bootstrap
> > Lines 72 (patched)
> > <https://reviews.apache.org/r/61172/diff/8/?file=1929289#file1929289line72>
> >
> >     This file does not exist, is that normal?

it should...
```
~/mesos/src/python/lib$ git blame requirements-test.in
456a0ac3 (Kevin Klues 2017-07-23 16:21:36 -0700 1) coverage
456a0ac3 (Kevin Klues 2017-07-23 16:21:36 -0700 2) mock
456a0ac3 (Kevin Klues 2017-07-23 16:21:36 -0700 3) pytest
456a0ac3 (Kevin Klues 2017-07-23 16:21:36 -0700 4) pytest-cov
```


> On Jan. 5, 2018, 3:23 p.m., Armand Grillet wrote:
> > src/python/lib/mesos/http.py
> > Lines 72 (patched)
> > <https://reviews.apache.org/r/61172/diff/8/?file=1929291#file1929291line72>
> >
> >     We should do that everywhere in the CLI, this is a useful addition. We could also use [mypy](http://www.mypy-lang.org/).

that looks pretty cool! we could also migrate to python3 which supports type annotations


- Eric


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


On Jan. 1, 2018, 1:52 a.m., Eric Chung wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61172/
> -----------------------------------------------------------
> 
> (Updated Jan. 1, 2018, 1:52 a.m.)
> 
> 
> Review request for mesos, Armand Grillet, Jason Lai, and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Part of MESOS-7310, this patch adds the mesos.http and mesos.exceptions
> modules, which provides a Resource class and its descendants for
> abstracting away common operations over http connectioins with JSON
> serialization.
> 
> updated patch according to klueska's recommendations
> 
> 
> addressed comments
> 
> 
> Diffs
> -----
> 
>   src/python/cli_new/bootstrap c84e549e59bc7a07b5b70c24c63bc0c16881e442 
>   src/python/lib/mesos/exceptions.py PRE-CREATION 
>   src/python/lib/mesos/http.py PRE-CREATION 
>   src/python/lib/requirements.in e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 
>   src/python/lib/tests/conftest.py PRE-CREATION 
>   src/python/lib/tests/test_exceptions.py PRE-CREATION 
>   src/python/lib/tests/test_http.py PRE-CREATION 
>   src/python/lib/tox.ini 8ad030d2dbf1fb7a04f1eaadb587462b4ef3f054 
>   support/pylint.config f74f553e238553bd6a6c06f4dd888cc5954a33eb 
> 
> 
> Diff: https://reviews.apache.org/r/61172/diff/8/
> 
> 
> Testing
> -------
> 
> install tox
> cd src/python/lib
> tox
> 
> 
> Thanks,
> 
> Eric Chung
> 
>

Re: Review Request 61172: Added mesos.http and mesos.exceptions for CLI.

Posted by Armand Grillet <ag...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61172/#review194862
-----------------------------------------------------------



Some small issues to fix, there are also some typos in the commit description.


src/python/cli_new/bootstrap
Lines 72 (patched)
<https://reviews.apache.org/r/61172/#comment273927>

    This file does not exist, is that normal?



src/python/lib/mesos/exceptions.py
Lines 26 (patched)
<https://reviews.apache.org/r/61172/#comment273928>

    s/mesos/Mesos



src/python/lib/mesos/exceptions.py
Lines 30 (patched)
<https://reviews.apache.org/r/61172/#comment273929>

    We use the syntax `"{msg}: {exception}".format(msg=message, exception=original_exception)` in the rest of our Python codebase. I do not have an opinion regarding the syntax but we should follow the current one in that review request and do a refactoring later if necessary.



src/python/lib/mesos/exceptions.py
Lines 60 (patched)
<https://reviews.apache.org/r/61172/#comment273930>

    s/'/"



src/python/lib/mesos/exceptions.py
Lines 75 (patched)
<https://reviews.apache.org/r/61172/#comment273931>

    



src/python/lib/mesos/exceptions.py
Lines 83 (patched)
<https://reviews.apache.org/r/61172/#comment273934>

    Unecessary comma.



src/python/lib/mesos/exceptions.py
Lines 106 (patched)
<https://reviews.apache.org/r/61172/#comment273935>

    "Given when " is unecessary.



src/python/lib/mesos/http.py
Lines 20 (patched)
<https://reviews.apache.org/r/61172/#comment273936>

    s/`restful API`/`RESTful API.`



src/python/lib/mesos/http.py
Lines 25 (patched)
<https://reviews.apache.org/r/61172/#comment273939>

    Let's do `from urlparse import urlparse` instead as we only use this once for that function in that file.



src/python/lib/mesos/http.py
Lines 33 (patched)
<https://reviews.apache.org/r/61172/#comment273937>

    Not in alphabetical order but it makes sense due to the inheritance of these classes.



src/python/lib/mesos/http.py
Lines 72 (patched)
<https://reviews.apache.org/r/61172/#comment273938>

    We should do that everywhere in the CLI, this is a useful addition. We could also use [mypy](http://www.mypy-lang.org/).



src/python/lib/mesos/http.py
Lines 117 (patched)
<https://reviews.apache.org/r/61172/#comment273944>

    Will just be `urlparse(url)` following previous comment.



src/python/lib/mesos/http.py
Lines 245 (patched)
<https://reviews.apache.org/r/61172/#comment273940>

    ```
    :param use_gzip_encoding: boolean indicating whether to pass gzip
                              encoding in the request headers or not
    ```



src/python/lib/mesos/http.py
Lines 261 (patched)
<https://reviews.apache.org/r/61172/#comment273941>

    I have some trouble reading this comment. I would write something like "We retry only when it makes sense: either due to a network partition (e.g. connection errors) or if the request failed due to a server error such as 500s, timeouts, and so on."



src/python/lib/tests/test_http.py
Lines 239 (patched)
<https://reviews.apache.org/r/61172/#comment273943>

    s/resrouce/resource


- Armand Grillet


On Jan. 1, 2018, 1:52 a.m., Eric Chung wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61172/
> -----------------------------------------------------------
> 
> (Updated Jan. 1, 2018, 1:52 a.m.)
> 
> 
> Review request for mesos, Armand Grillet, Jason Lai, and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Part of MESOS-7310, this patch adds the mesos.http and mesos.exceptions
> modules, which provides a Resource class and its descendants for
> abstracting away common operations over http connectioins with JSON
> serialization.
> 
> updated patch according to klueska's recommendations
> 
> 
> addressed comments
> 
> 
> Diffs
> -----
> 
>   src/python/cli_new/bootstrap c84e549e59bc7a07b5b70c24c63bc0c16881e442 
>   src/python/lib/mesos/exceptions.py PRE-CREATION 
>   src/python/lib/mesos/http.py PRE-CREATION 
>   src/python/lib/requirements.in e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 
>   src/python/lib/tests/conftest.py PRE-CREATION 
>   src/python/lib/tests/test_exceptions.py PRE-CREATION 
>   src/python/lib/tests/test_http.py PRE-CREATION 
>   src/python/lib/tox.ini 8ad030d2dbf1fb7a04f1eaadb587462b4ef3f054 
>   support/pylint.config f74f553e238553bd6a6c06f4dd888cc5954a33eb 
> 
> 
> Diff: https://reviews.apache.org/r/61172/diff/8/
> 
> 
> Testing
> -------
> 
> install tox
> cd src/python/lib
> tox
> 
> 
> Thanks,
> 
> Eric Chung
> 
>