> On Sept. 14, 2015, 11:01 a.m., Kevin Sweeney wrote: > > commons/src/main/java/org/apache/aurora/common/net/http/handlers/AbortHandler.java, > > lines 72-73 > > <https://reviews.apache.org/r/38332/diff/2/?file=1071120#file1071120line72> > > > > Will this ever appear to the client? Presumably abortListener shuts > > down the http server. quitquitquit appears to get around this by forking a > > new thread for the listener.
Yeah, i thought through thist oo...it's all but certain to not show up. I've changed this to own up and return nothing. > On Sept. 14, 2015, 11:01 a.m., Kevin Sweeney wrote: > > commons/src/main/java/org/apache/aurora/common/net/http/handlers/ContentionPrinter.java, > > line 83 > > <https://reviews.apache.org/r/38332/diff/2/?file=1071122#file1071122line83> > > > > `String.join` Done. > On Sept. 14, 2015, 11:01 a.m., Kevin Sweeney wrote: > > commons/src/main/java/org/apache/aurora/common/net/http/handlers/QuitHandler.java, > > lines 72-73 > > <https://reviews.apache.org/r/38332/diff/2/?file=1071125#file1071125line72> > > > > same question - will this actually run? Probably, but not guaranteed. In practice, teardown is is indefinite and much less predictable than the abort. > On Sept. 14, 2015, 11:01 a.m., Kevin Sweeney wrote: > > commons/src/test/java/org/apache/aurora/common/net/http/handlers/VarsHandlerTest.java, > > line 77 > > <https://reviews.apache.org/r/38332/diff/2/?file=1071133#file1071133line77> > > > > String.join Done. > On Sept. 14, 2015, 11:01 a.m., Kevin Sweeney wrote: > > commons/src/main/java/org/apache/aurora/common/net/http/handlers/TimeSeriesDataSource.java, > > line 101 > > <https://reviews.apache.org/r/38332/diff/2/?file=1071129#file1071129line101> > > > > isn't the `@Produces` annotation enough here? afaik we don't need to > > explicitly serialize and deserialize json in jax-rs due to the json provider I'd like to stay away from altering the implementation - this would pull a thread into unit tests, so i've left a TODO to defer. > On Sept. 14, 2015, 11:01 a.m., Kevin Sweeney wrote: > > commons/src/main/java/org/apache/aurora/common/net/http/handlers/VarsJsonHandler.java, > > line 72 > > <https://reviews.apache.org/r/38332/diff/2/?file=1071131#file1071131line72> > > > > same question - do we stilll need to explicitly serialize to json? Ditto - not necessary, but useful to limit the scope of the diff. TODO added. > On Sept. 14, 2015, 11:01 a.m., Kevin Sweeney wrote: > > src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java, > > lines 203-212 > > <https://reviews.apache.org/r/38332/diff/2/?file=1071135#file1071135line203> > > > > This feels like a DRY violation to me - consider adding > > > > `public static final String PATH = "/api"` > > > > etc to each resource and referencing them in both the `@Path` > > annotation and here. It is indeed a DRY violation, but it's not a new one - the existing path specs have this problem. There are some nuances to differences between the `@Path` value and the filter value here, which leads me to prefer tackling an improved pattern separately. Also, please see my reply to jcohen above - ultimately i'd like to find a way out of this filtering business and let jax-rs annotations Just Work with servlets, which would also address the DRY violation. - Bill ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/38332/#review98877 ----------------------------------------------------------- On Sept. 13, 2015, 11:06 a.m., Bill Farner wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/38332/ > ----------------------------------------------------------- > > (Updated Sept. 13, 2015, 11:06 a.m.) > > > Review request for Aurora and Joshua Cohen. > > > Repository: aurora > > > Description > ------- > > There are a few positive outcomes here: > > * our HTTP serving code is much more uniform > * endpoints have less boilerplate code > * endpoints are easier to test > * ServletModule setup is simpler and uniform > > > Diffs > ----- > > buildSrc/src/main/groovy/org/apache/aurora/build/CoverageReportCheck.groovy > b996d4597492a76db0f2571db4e6c1ae9b4bcf33 > > commons/src/main/java/org/apache/aurora/common/net/http/handlers/AbortHandler.java > e97bd825752bf04cca391e2119b5e33c66a1cab5 > > commons/src/main/java/org/apache/aurora/common/net/http/handlers/AssetHandler.java > 7a44f07e8514f32637d1832273cb4d9081a14031 > > commons/src/main/java/org/apache/aurora/common/net/http/handlers/ContentionPrinter.java > 1f8c453c8e582da44849a8e922d974a0e054c638 > > commons/src/main/java/org/apache/aurora/common/net/http/handlers/HealthHandler.java > cc5ad4d5e15eaff78832c033e77b4ac0a532f6b3 > > commons/src/main/java/org/apache/aurora/common/net/http/handlers/LogConfig.java > 5520fb6a83d37884f6cace995f3cc3313c3980c4 > > commons/src/main/java/org/apache/aurora/common/net/http/handlers/QuitHandler.java > 4ce3c971b8e8ca696d7d5ea4e7d348237b836513 > > commons/src/main/java/org/apache/aurora/common/net/http/handlers/StringTemplateServlet.java > 60e0abb7f7bac4c84ece5e007a4b3980fbd9a585 > > commons/src/main/java/org/apache/aurora/common/net/http/handlers/TextResponseHandler.java > 23068eb4b8d9f1d848b4d007e14e7d32f3189ee1 > > commons/src/main/java/org/apache/aurora/common/net/http/handlers/ThreadStackPrinter.java > 5dd88041faafc563c87f51807476aa142e4942d5 > > commons/src/main/java/org/apache/aurora/common/net/http/handlers/TimeSeriesDataSource.java > e87fe2c2a2d2ed609273298eb57085bcb155c3b2 > > commons/src/main/java/org/apache/aurora/common/net/http/handlers/VarsHandler.java > bf04525115fa4045e792f2c45116e8d10addb24f > > commons/src/main/java/org/apache/aurora/common/net/http/handlers/VarsJsonHandler.java > e97ec6085c52a155ff9978c5121a5f98a8f93593 > > commons/src/test/java/org/apache/aurora/common/net/http/handlers/AssetHandlerTest.java > 740c42fedf668233b80f878728620a10ecf9b86f > > commons/src/test/java/org/apache/aurora/common/net/http/handlers/VarsHandlerTest.java > 34f62fb9db2d723f8ef171db7c7b485d8416d24f > config/legacy_untested_classes.txt 07fd5f1066a4d926072d91b9de170b9035fb3ea4 > src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java > c503dcceb417d1f73c30dc63cf5352470d4c761d > src/test/java/org/apache/aurora/scheduler/app/local/LocalSchedulerMain.java > 738c8b644287b93372f3227832a9d92b95dc498a > > Diff: https://reviews.apache.org/r/38332/diff/ > > > Testing > ------- > > manually clicked through everything i could think of in ./gradlew run and in > vagrant > end-to-end tests > > > Thanks, > > Bill Farner > >
