----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/64362/#review192977 -----------------------------------------------------------
Ship it! Wow, pretty nice! I feel your pain. I also tried once to update Guice and failed miserably. build.gradle Lines 375-377 (original), 384-386 (patched) <https://reviews.apache.org/r/64362/#comment271414> License is Apache 2.0, so this looks good. https://github.com/resteasy/Resteasy/blob/master/License.html src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java Line 211 (original), 218 (patched) <https://reviews.apache.org/r/64362/#comment271413> To fail or to pass? The new guice version is great! Running `./gradlew jmh -Pbenchmarks='SnapshotBenchmarks.*'` with master returns this error that gives no real clue how to fix it: ``` com.google.inject.internal.util.$ComputationException: java.lang.ArrayIndexOutOfBoundsException: 22081 at com.google.inject.internal.util.$MapMaker$StrategyImpl.compute(MapMaker.java:553) at com.google.inject.internal.util.$MapMaker$StrategyImpl.compute(MapMaker.java:419) at com.google.inject.internal.util.$CustomConcurrentHashMap$ComputingImpl.get(CustomConcurrentHashMap.java:2041) at com.google.inject.internal.util.$StackTraceElements.forMember(StackTraceElements.java:53) at com.google.inject.internal.Errors.formatInjectionPoint(Errors.java:716) at com.google.inject.internal.Errors.formatSource(Errors.java:678) at com.google.inject.internal.Errors.format(Errors.java:555) at com.google.inject.CreationException.getMessage(CreationException.java:48) at java.lang.Throwable.getLocalizedMessage(Throwable.java:391) at java.lang.Throwable.toString(Throwable.java:480) at java.lang.Throwable.<init>(Throwable.java:311) at java.lang.Exception.<init>(Exception.java:102) at java.lang.RuntimeException.<init>(RuntimeException.java:96) at org.openjdk.jmh.runner.BenchmarkException.<init>(BenchmarkException.java:34) at org.openjdk.jmh.runner.BenchmarkHandler.runIteration(BenchmarkHandler.java:438) at org.openjdk.jmh.runner.BaseRunner.runBenchmark(BaseRunner.java:263) at org.openjdk.jmh.runner.BaseRunner.runBenchmark(BaseRunner.java:235) at org.openjdk.jmh.runner.BaseRunner.doSingle(BaseRunner.java:142) at org.openjdk.jmh.runner.BaseRunner.runBenchmarksForked(BaseRunner.java:76) at org.openjdk.jmh.runner.ForkedRunner.run(ForkedRunner.java:72) at org.openjdk.jmh.runner.ForkedMain.main(ForkedMain.java:84) Caused by: java.lang.ArrayIndexOutOfBoundsException: 22081 at com.google.inject.internal.asm.$ClassReader.<init>(Unknown Source) at com.google.inject.internal.asm.$ClassReader.<init>(Unknown Source) at com.google.inject.internal.asm.$ClassReader.<init>(Unknown Source) at com.google.inject.internal.util.$LineNumbers.<init>(LineNumbers.java:62) at com.google.inject.internal.util.$StackTraceElements$1.apply(StackTraceElements.java:36) at com.google.inject.internal.util.$StackTraceElements$1.apply(StackTraceElements.java:33) at com.google.inject.internal.util.$MapMaker$StrategyImpl.compute(MapMaker.java:549) ... 20 more ``` Running the same benchmark with this patch: ``` com.google.inject.CreationException: Unable to create injector, see the following errors: 1) No implementation for org.apache.aurora.scheduler.TierManager was bound. while locating org.apache.aurora.scheduler.TierManager for the 1st parameter of org.apache.aurora.scheduler.storage.durability.ThriftBackfill.<init>(ThriftBackfill.java:56) while locating org.apache.aurora.scheduler.storage.durability.ThriftBackfill for the 4th parameter of org.apache.aurora.scheduler.storage.log.SnapshotStoreImpl.<init>(SnapshotStoreImpl.java:267) at org.apache.aurora.benchmark.SnapshotBenchmarks$RestoreSnapshotWithUpdatesBenchmark$1.configure(SnapshotBenchmarks.java:90) 2) No implementation for org.apache.aurora.scheduler.storage.Storage annotated with @org.apache.aurora.scheduler.storage.Storage$Volatile() was bound. while locating org.apache.aurora.scheduler.storage.Storage annotated with @org.apache.aurora.scheduler.storage.Storage$Volatile() for the 3rd parameter of org.apache.aurora.scheduler.storage.log.SnapshotStoreImpl.<init>(SnapshotStoreImpl.java:267) at org.apache.aurora.benchmark.SnapshotBenchmarks$RestoreSnapshotWithUpdatesBenchmark$1.configure(SnapshotBenchmarks.java:90) 2 errors at com.google.inject.internal.Errors.throwCreationExceptionIfErrorsExist(Errors.java:470) at com.google.inject.internal.InternalInjectorCreator.initializeStatically(InternalInjectorCreator.java:155) at com.google.inject.internal.InternalInjectorCreator.build(InternalInjectorCreator.java:107) at com.google.inject.Guice.createInjector(Guice.java:99) at com.google.inject.Guice.createInjector(Guice.java:73) at com.google.inject.Guice.createInjector(Guice.java:62) at org.apache.aurora.benchmark.SnapshotBenchmarks$RestoreSnapshotWithUpdatesBenchmark.getSnapshotStore(SnapshotBenchmarks.java:84) at org.apache.aurora.benchmark.SnapshotBenchmarks$RestoreSnapshotWithUpdatesBenchmark.setUp(SnapshotBenchmarks.java:68) at org.apache.aurora.benchmark.generated.SnapshotBenchmarks_RestoreSnapshotWithUpdatesBenchmark_run_jmhTest._jmh_tryInit_f_restoresnapshotwithupdatesbenchmark0_0(SnapshotBenchmarks_RestoreSnapshotWithUpdatesBenchmark_run_jmhTest.java:334) at org.apache.aurora.benchmark.generated.SnapshotBenchmarks_RestoreSnapshotWithUpdatesBenchmark_run_jmhTest.run_Throughput(SnapshotBenchmarks_RestoreSnapshotWithUpdatesBenchmark_run_jmhTest.java:69) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:498) at org.openjdk.jmh.runner.BenchmarkHandler$BenchmarkTask.call(BenchmarkHandler.java:464) at org.openjdk.jmh.runner.BenchmarkHandler$BenchmarkTask.call(BenchmarkHandler.java:448) at java.util.concurrent.FutureTask.run(FutureTask.java:266) at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511) at java.util.concurrent.FutureTask.run(FutureTask.java:266) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) at java.lang.Thread.run(Thread.java:748) ``` - Stephan Erb On Dec. 6, 2017, 8:36 a.m., Bill Farner wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/64362/ > ----------------------------------------------------------- > > (Updated Dec. 6, 2017, 8:36 a.m.) > > > Review request for Aurora, David McLaughlin, Jordan Ly, and Stephan Erb. > > > Repository: aurora > > > Description > ------- > > Upgrading guice was the original goal with this patch, which pulled along > several other dependencies. Guice 3 [suffers from obscure > errors](https://github.com/google/guice/issues/757) when creating binding > error messages with java 8 and lambdas. This has been a frequent source of > frustration since we first upgraded to java 8 mid-2015. > > I've gone spelunking down this path numerous times, and frequently hit a wall > with jersey. We needed to upgrade jersey-guice, to upgrade jersey, to > upgrade guice. jersey introduced their own dependency injection (HK2) in > jersey 2.0, which complicated matters. There have been some promising > developments since (hk2-guice > [bridge](https://javaee.github.io/hk2/guice-bridge.html#bi-directional-hk2-guice-bridge), > 2.26 [abstracted HK2](https://jersey.github.io/release-notes/2.26.html), and > several [projects](https://github.com/Squarespace/jersey2-guice) have emerged > to solve the issue). Unfortunately, each avenue failed with some combination > of not working well with our application design, or i just plain couldn't get > it working. I began to look beyond jersey. > > This left restlet and resteasy as the most common alternatives. I balked > early at restlet due to their guice integration being > [apparently](https://github.com/restlet/restlet-framework-java/commits/master/modules/org.restlet.ext.guice) > > [dormant](https://stackoverflow.com/questions/8227583/what-is-the-status-of-org-restlet-ext-guice). > > Fortunately i achieved some early wins with resteasy! Migrating was > straightforward with a small patch based on some examples. > > However, i hit a hurdle with shiro-guice. It [needed to be > updated](https://issues.apache.org/jira/browse/SHIRO-493) to work with guice > 4, and there were some necessary API changes. No big deal, just the > `filterConfig()` nesting you see in this patch. This revealed a deeper issue > with binding custom `Filter`s with `ShiroWebModule`. Previously, > `ShiroWebModule` would effectively only `bind()` keys [they > define](https://github.com/apache/shiro/blob/f326fd3814f672464deb11c3dadadb27af618eec/support/guice/src/main/java/org/apache/shiro/guice/web/ShiroWebModule.java#L65-L86), > allowing the API user to `bind()` custom keys. The > [patch](https://github.com/apache/shiro/commit/f2dfa7ff39c9870e7b9856ceca8690c5398080fa#diff-359a7b20d3b7fdcc0ffce11ad57d6e1c) > to support guice 4 changed that, and `bind()` will be > [called](https://github.com/apache/shiro/commit/f2dfa7ff39c9870e7b9856ceca8690c5398080fa#diff-359a7b20d3b7fdcc0ffce11ad57d6e1cR183) > on these custom keys. In our case, this caused a duplicate binding. > > The simplest workaround to this was to avoid `bind()`ing the custom > `afterAuthFilter` key, and use the custom type as the key type (e.g. > `Key.get(CountingFilter.class)` rather than `Key.get(Filter.class)`). > > Lastly, `GuiceResteasyBootstrapServletContextListener` does not integrate > with `GuiceServletContextListener` in the way `GuiceFilter` > [demands](https://github.com/google/guice/blob/e7bef34ef6379735e2a58df1b23f351bb7e30e44/extensions/servlet/src/com/google/inject/servlet/ServletModule.java#L322-L323), > which necessitated the passing of `ServletContext` you see in this patch. > > I can't say i'm happy with the outcome here, but i am overall happier given > that guice is upgraded. > > Computers are hard! > > > Diffs > ----- > > build.gradle fd606a27bbe0f79a9358ddb74425437ddb42d71e > src/main/java/org/apache/aurora/scheduler/http/JettyServerModule.java > 0f8528c3403b5f51f082aa54c16358f7568f439a > src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java > a19663acb20c66ec14e41473a72fa5035146a6cc > > src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java > d81671c0bf3634bfce851937f4fc9135dd5f563f > src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java > a3f6941f6b335791455545e0745b03cf58899f95 > src/test/java/org/apache/aurora/scheduler/http/api/ApiBetaTest.java > e8ef6bd7f9b0c47c5f17da7a040ce1497922a807 > src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java > 43fa315a9a25bf29f53d8fd986f198d3f9ef1a74 > > src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java > d3c7ac985bbcf734aace2cad50af970619ffb536 > > src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroKerberosAuthenticationFilterTest.java > 20e3d6255921cf24da7cf66101a72c537a0a7c68 > > src/test/java/org/apache/aurora/scheduler/http/api/security/ShiroKerberosPermissiveAuthenticationFilterTest.java > 001846765f8f9e8b970ab3b3d2a495652819319c > > > Diff: https://reviews.apache.org/r/64362/diff/1/ > > > Testing > ------- > > end-to-end tests are broken on master, will fix separately before landing > this patch > > > Thanks, > > Bill Farner > >
