Re: Review Request 42656: Don't destroy session between requests with TRequestsTransport

2016-03-07 Thread Aurora ReviewBot 

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


Ship it!




Master (a91a759) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On March 7, 2016, 7:38 p.m., Kunal Thakar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42656/
> ---
> 
> (Updated March 7, 2016, 7:38 p.m.)
> 
> 
> Review request for Aurora and Stephan Erb.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> As an API consumer, I want the API client to reuse open connections and not 
> create a new one for every query.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/common/transport.py 
> 909021ac203185862267d4359d332fc169a06b7e 
>   src/test/python/apache/aurora/common/test_transport.py 
> 1f589a9ae08e1f13be34ad6002ceb11a43fdeb5f 
> 
> Diff: https://reviews.apache.org/r/42656/diff/
> 
> 
> Testing
> ---
> 
> ./pants test.pytest src/test/python::
> 
> 
> Thanks,
> 
> Kunal Thakar
> 
>

Re: Review Request 42656: Don't destroy session between requests with TRequestsTransport

2016-03-07 Thread Kunal Thakar 

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

(Updated March 7, 2016, 7:38 p.m.)


Review request for Aurora and Stephan Erb.


Changes
---

I have added the isOpen() check back to flush as it was breaking unit tests 
that did not explicitly call transport.open() before making client calls. I can 
fix the unit tests, but I think this is safer.

On a related note, it looks like this code was copied from THttpClient from the 
thrift repo 
(https://github.com/apache/thrift/blob/master/lib/py/src/transport/THttpClient.py#L115),
 where it made sense to open a new connection with every flush, but in our 
case, it doesn't make sense.


Repository: aurora


Description
---

As an API consumer, I want the API client to reuse open connections and not 
create a new one for every query.


Diffs (updated)
-

  src/main/python/apache/aurora/common/transport.py 
909021ac203185862267d4359d332fc169a06b7e 
  src/test/python/apache/aurora/common/test_transport.py 
1f589a9ae08e1f13be34ad6002ceb11a43fdeb5f 

Diff: https://reviews.apache.org/r/42656/diff/


Testing
---

./pants test.pytest src/test/python::


Thanks,

Kunal Thakar

Re: Review Request 42656: Don't destroy session between requests with TRequestsTransport

2016-02-18 Thread Stephan Erb 

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



The tests are failing with my proposed change. I will have to re-check how I 
could have missed that...

- Stephan Erb


On Feb. 18, 2016, 1:01 a.m., Kunal Thakar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42656/
> ---
> 
> (Updated Feb. 18, 2016, 1:01 a.m.)
> 
> 
> Review request for Aurora and Stephan Erb.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> As an API consumer, I want the API client to reuse open connections and not 
> create a new one for every query.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/common/transport.py 
> 909021ac203185862267d4359d332fc169a06b7e 
>   src/test/python/apache/aurora/common/test_transport.py 
> 1f589a9ae08e1f13be34ad6002ceb11a43fdeb5f 
> 
> Diff: https://reviews.apache.org/r/42656/diff/
> 
> 
> Testing
> ---
> 
> ./pants test.pytest src/test/python::
> 
> 
> Thanks,
> 
> Kunal Thakar
> 
>

Re: Review Request 42656: Don't destroy session between requests with TRequestsTransport

2016-02-17 Thread Aurora ReviewBot 

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



Master (9d642dc) is red with this patch.
  ./build-support/jenkins/build.sh

   handler = ReadOnlySchedulerHandler()
   processor = 
ReadOnlyScheduler.Processor(handler)
   pfactory = 
TJSONProtocol.TJSONProtocolFactory()
   server = THttpServer.THttpServer(processor, 
('localhost', 0), pfactory)
   server_thread = Thread(target=server.serve)
   server_thread.start()
   _, server_port = 
server.httpd.socket.getsockname()
 
   try:
 transport = 
TRequestsTransport('http://localhost:%d' % server_port)
 protocol = 
TJSONProtocol.TJSONProtocol(transport)
 client = ReadOnlyScheduler.Client(protocol)
 >   client.getRoleSummary()
 
 
src/test/python/apache/aurora/common/test_transport.py:194: 
 _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
 
.pants.d/python-setup/chroots/62842f64d44dfb8a1ac3e9b3ead8f6af1e207916/.deps/api.src.main.thrift.org.apache.aurora.gen._api_c7736d29-0.0.0-py2-none-any.whl/gen/apache/aurora/api/ReadOnlyScheduler.py:147:
 in getRoleSummary
 self.send_getRoleSummary()
 
.pants.d/python-setup/chroots/62842f64d44dfb8a1ac3e9b3ead8f6af1e207916/.deps/api.src.main.thrift.org.apache.aurora.gen._api_c7736d29-0.0.0-py2-none-any.whl/gen/apache/aurora/api/ReadOnlyScheduler.py:155:
 in send_getRoleSummary
 self._oprot.trans.flush()
 _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
 
 self = 
 
 def flush(self):
   data = self.__wbuf.getvalue()
   self.__wbuf = BytesIO()
 
 > self._session.headers['Content-Type'] = 
'application/x-thrift'
 E AttributeError: 'NoneType' object has no 
attribute 'headers'
 
 
.pants.d/python-setup/chroots/62842f64d44dfb8a1ac3e9b3ead8f6af1e207916/apache/aurora/common/transport.py:117:
 AttributeError
  generated xml file: 
/home/jenkins/jenkins-slave/workspace/AuroraBot/dist/test-results/415337499eb72578eab327a6487c1f5c9452b3d6.xml
 
  7 failed, 655 passed, 5 skipped, 1 warnings in 
332.52 seconds 
 
FAILURE


00:19:32 06:29   [complete]
   FAILURE


I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Feb. 18, 2016, 12:01 a.m., Kunal Thakar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42656/
> ---
> 
> (Updated Feb. 18, 2016, 12:01 a.m.)
> 
> 
> Review request for Aurora and Stephan Erb.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> As an API consumer, I want the API client to reuse open connections and not 
> create a new one for every query.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/common/transport.py 
> 909021ac203185862267d4359d332fc169a06b7e 
>   src/test/python/apache/aurora/common/test_transport.py 
> 1f589a9ae08e1f13be34ad6002ceb11a43fdeb5f 
> 
> Diff: https://reviews.apache.org/r/42656/diff/
> 
> 
> Testing
> ---
> 
> ./pants test.pytest src/test/python::
> 
> 
> Thanks,
> 
> Kunal Thakar
> 
>

Re: Review Request 42656: Don't destroy session between requests with TRequestsTransport

2016-02-17 Thread Kunal Thakar 

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

(Updated Feb. 18, 2016, 12:01 a.m.)


Review request for Aurora and Stephan Erb.


Changes
---

Removed open/close entirely from flush


Repository: aurora


Description
---

As an API consumer, I want the API client to reuse open connections and not 
create a new one for every query.


Diffs (updated)
-

  src/main/python/apache/aurora/common/transport.py 
909021ac203185862267d4359d332fc169a06b7e 
  src/test/python/apache/aurora/common/test_transport.py 
1f589a9ae08e1f13be34ad6002ceb11a43fdeb5f 

Diff: https://reviews.apache.org/r/42656/diff/


Testing
---

./pants test.pytest src/test/python::


Thanks,

Kunal Thakar

Re: Review Request 42656: Don't destroy session between requests with TRequestsTransport

2016-02-16 Thread Stephan Erb 

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


Fix it, then Ship it!




I ran some tests. I have dropped the open/close handling from `flush` and then:

* run the Aurora client tests
* run our internal test suite for our client-wrapper
* set up a long lasting client connection to the scheduler and then did a 
failover by stopping the current leading scheduler.

I did not notice any problems. I believe this change is therefore good to go.

Disclaimer: If there was a performance improvement, it did not surface in our 
tests.


src/main/python/apache/aurora/common/transport.py (line 114)


I believe this whole open/close handling can be dropped from `flush`. I did 
not run into problems even when running with `assert self.isOpen()`


- Stephan Erb


On Jan. 22, 2016, 8:29 p.m., Kunal Thakar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42656/
> ---
> 
> (Updated Jan. 22, 2016, 8:29 p.m.)
> 
> 
> Review request for Aurora and Joshua Cohen.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> As an API consumer, I want the API client to reuse open connections and not 
> create a new one for every query.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/common/transport.py 
> 909021ac203185862267d4359d332fc169a06b7e 
>   src/test/python/apache/aurora/common/test_transport.py 
> 1f589a9ae08e1f13be34ad6002ceb11a43fdeb5f 
> 
> Diff: https://reviews.apache.org/r/42656/diff/
> 
> 
> Testing
> ---
> 
> ./pants test.pytest src/test/python::
> 
> 
> Thanks,
> 
> Kunal Thakar
> 
>

Re: Review Request 42656: Don't destroy session between requests with TRequestsTransport

2016-01-22 Thread Aurora ReviewBot 

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

Ship it!


Master (66a4d5f) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Jan. 22, 2016, 4:58 p.m., Kunal Thakar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42656/
> ---
> 
> (Updated Jan. 22, 2016, 4:58 p.m.)
> 
> 
> Review request for Aurora and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> As an API consumer, I want the API client to reuse open connections and not 
> create a new one for every query.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/common/transport.py 
> 909021ac203185862267d4359d332fc169a06b7e 
>   src/test/python/apache/aurora/common/test_transport.py 
> 1f589a9ae08e1f13be34ad6002ceb11a43fdeb5f 
> 
> Diff: https://reviews.apache.org/r/42656/diff/
> 
> 
> Testing
> ---
> 
> ./pants test.pytest src/test/python::
> 
> 
> Thanks,
> 
> Kunal Thakar
> 
>

Review Request 42656: Don't destroy session between requests with TRequestsTransport

2016-01-22 Thread Kunal Thakar 

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

Review request for Aurora.


Repository: aurora


Description
---

As an API consumer, I want the API client to reuse open connections and not 
create a new one for every query.


Diffs
-

  src/main/python/apache/aurora/common/transport.py 
909021ac203185862267d4359d332fc169a06b7e 
  src/test/python/apache/aurora/common/test_transport.py 
1f589a9ae08e1f13be34ad6002ceb11a43fdeb5f 

Diff: https://reviews.apache.org/r/42656/diff/


Testing
---

./pants test.pytest src/test/python::


Thanks,

Kunal Thakar