Re: Review Request 41453: HTTP server cleanup - shut down the server after unit tests, remove jetty bug workaround.

2015-12-16 Thread John Sirois 

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


Drive-by


src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java (line 148)


It looks like this can be an NPE without advanced knowledge of how the base 
class is used in practice, complicated by the fact the field is set inside a 
try - seems the setting could be done outside the try, but ...



src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java (line 156)


The Optional indirection (+ volatile bit) seems a bit odd - the 
startupService is never absent.  How about just keep the variable local and 
non-Optional and on the next line add the stopAsync as a tear down?


- John Sirois


On Dec. 16, 2015, 11:19 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41453/
> ---
> 
> (Updated Dec. 16, 2015, 11:19 a.m.)
> 
> 
> Review request for Aurora and Joshua Cohen.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> HTTP server cleanup - shut down the server after unit tests, remove jetty bug 
> workaround.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 
> 838bfc9ed6242e777d81a17b337f342b7bea72ec 
>   src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java 
> 5768481005ef505d6c397101d8cc005af9d6815f 
> 
> Diff: https://reviews.apache.org/r/41453/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 41453: HTTP server cleanup - shut down the server after unit tests, remove jetty bug workaround.

2015-12-16 Thread Joshua Cohen 

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

Ship it!


Ship It!

- Joshua Cohen


On Dec. 16, 2015, 7:53 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41453/
> ---
> 
> (Updated Dec. 16, 2015, 7:53 p.m.)
> 
> 
> Review request for Aurora and Joshua Cohen.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> HTTP server cleanup - shut down the server after unit tests, remove jetty bug 
> workaround.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 
> 838bfc9ed6242e777d81a17b337f342b7bea72ec 
>   src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java 
> 5768481005ef505d6c397101d8cc005af9d6815f 
> 
> Diff: https://reviews.apache.org/r/41453/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 41453: HTTP server cleanup - shut down the server after unit tests, remove jetty bug workaround.

2015-12-16 Thread John Sirois 

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

Ship it!


Ship It!

- John Sirois


On Dec. 16, 2015, 12:53 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41453/
> ---
> 
> (Updated Dec. 16, 2015, 12:53 p.m.)
> 
> 
> Review request for Aurora and Joshua Cohen.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> HTTP server cleanup - shut down the server after unit tests, remove jetty bug 
> workaround.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 
> 838bfc9ed6242e777d81a17b337f342b7bea72ec 
>   src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java 
> 5768481005ef505d6c397101d8cc005af9d6815f 
> 
> Diff: https://reviews.apache.org/r/41453/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 41453: HTTP server cleanup - shut down the server after unit tests, remove jetty bug workaround.

2015-12-16 Thread Bill Farner 

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



src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 


This bug has apparently been fixed on the jetty side:

> Good. Although your problem is fixed with the 
Resource.setDefaultUseCaches,  I am marking this bug as "fixed", as I've also 
modified the code in a few places that was doing direct Url.openStream.

https://bugs.eclipse.org/bugs/show_bug.cgi?id=364936


- Bill Farner


On Dec. 16, 2015, 10:19 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41453/
> ---
> 
> (Updated Dec. 16, 2015, 10:19 a.m.)
> 
> 
> Review request for Aurora and Joshua Cohen.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> HTTP server cleanup - shut down the server after unit tests, remove jetty bug 
> workaround.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 
> 838bfc9ed6242e777d81a17b337f342b7bea72ec 
>   src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java 
> 5768481005ef505d6c397101d8cc005af9d6815f 
> 
> Diff: https://reviews.apache.org/r/41453/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 41453: HTTP server cleanup - shut down the server after unit tests, remove jetty bug workaround.

2015-12-16 Thread Aurora ReviewBot 

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

Ship it!


Master (fb8155d) 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 Dec. 16, 2015, 7:53 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41453/
> ---
> 
> (Updated Dec. 16, 2015, 7:53 p.m.)
> 
> 
> Review request for Aurora and Joshua Cohen.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> HTTP server cleanup - shut down the server after unit tests, remove jetty bug 
> workaround.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 
> 838bfc9ed6242e777d81a17b337f342b7bea72ec 
>   src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java 
> 5768481005ef505d6c397101d8cc005af9d6815f 
> 
> Diff: https://reviews.apache.org/r/41453/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 41453: HTTP server cleanup - shut down the server after unit tests, remove jetty bug workaround.

2015-12-16 Thread Bill Farner 


> On Dec. 16, 2015, 11:29 a.m., John Sirois wrote:
> > src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java, line 
> > 148
> > 
> >
> > It looks like this can be an NPE without advanced knowledge of how the 
> > base class is used in practice, complicated by the fact the field is set 
> > inside a try - seems the setting could be done outside the try, but ...

Yeah, this patch was a bit scatterbrained.  Thanks for pushing back.


> On Dec. 16, 2015, 11:29 a.m., John Sirois wrote:
> > src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java, line 
> > 156
> > 
> >
> > The Optional indirection (+ volatile bit) seems a bit odd - the 
> > startupService is never absent.  How about just keep the variable local and 
> > non-Optional and on the next line add the stopAsync as a tear down?

Totally forgot about teardown, that's a much better approach.


- Bill


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


On Dec. 16, 2015, 10:19 a.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41453/
> ---
> 
> (Updated Dec. 16, 2015, 10:19 a.m.)
> 
> 
> Review request for Aurora and Joshua Cohen.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> HTTP server cleanup - shut down the server after unit tests, remove jetty bug 
> workaround.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 
> 838bfc9ed6242e777d81a17b337f342b7bea72ec 
>   src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java 
> 5768481005ef505d6c397101d8cc005af9d6815f 
> 
> Diff: https://reviews.apache.org/r/41453/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Review Request 41453: HTTP server cleanup - shut down the server after unit tests, remove jetty bug workaround.

2015-12-16 Thread Bill Farner 

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

Review request for Aurora and Joshua Cohen.


Repository: aurora


Description
---

HTTP server cleanup - shut down the server after unit tests, remove jetty bug 
workaround.


Diffs
-

  src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java 
838bfc9ed6242e777d81a17b337f342b7bea72ec 
  src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java 
5768481005ef505d6c397101d8cc005af9d6815f 

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


Testing
---


Thanks,

Bill Farner