Use an embedded Jetty in ExoneraTor

added component::metrics/exonerator metrics-2017 owner::iwakeh priority::medium resolution::fixed severity::normal status::closed type::enhancement labels

+1

Not sure if it makes a difference, but we can likely avoid using JSPs in ExoneraTor and just use servlets. Not yet, but we can change things accordingly. (We'll still have to use JSPs in metrics-web.)

Have this working, now testing more thouroughly while working on #24175 (moved).

Trac:
Status: new to accepted
Owner: metrics-team to iwakeh

Please review this patch.

For my local tests (as I don't have a filled db locally) I used the war created by ant smoke-test-war, which only differs by the omitted db resource. Starting is done via java -jar generated/exonerator-dummy-test.war or java -jar generated/dist/exonerator-1.0.0-dev.war. The default port is 8080 and can be changed by setting system property 'exonerator.jetty.port', e.g. -Dexonerator.jetty.port=8888.

(The README and other build information have to be adapted in #24112 (moved).)

Trac:
Status: accepted to needs_review

+    <include name="jetty9-apache-jsp${jetty.version}-tweaked.jar" />
+    <include name="tomcat8-embed-jasper-8.5.14.jar" />
+    <include name="tomcat8-embed-el-8.5.14.jar" />
+    <include name="tomcat8-embed-core-8.5.14.jar" />
+    <include name="eclipse-ecj-3.11.1.jar" />

Yikes! That looks like a lot of duct tape! Much more than I had expected.

Let me suggest something totally different: Should we give up on JSPs now and migrate to facelets or whatever technology is recommended and supported these days?

The additional dependencies are not such a problematic thing. Simply tomcat embedded for jsps plus the eclipse compiler. All available as debian packages. Here the package listing: libasm-java (5.2-2) libtomcat8-embed-java (8.5.14-1+deb9u2) libecj-java (3.11.1-1)

Most other technologies won't have less dependencies.

BTW, the tweaked jar only prevents a duplicate include of a service definiton that wouldn't cause harm, so a rather picky measurement.

Replying to iwakeh:

The additional dependencies are not such a problematic thing. Simply tomcat embedded for jsps plus the eclipse compiler. All available as debian packages.

Different question then: if we need to include parts of embedded Tomcat, why don't we switch to that entirely? It's the embedded part that we're after, right? Or what else do we gain from picking Jetty in particular?

Here the package listing: libasm-java (5.2-2) libtomcat8-embed-java (8.5.14-1+deb9u2) libecj-java (3.11.1-1)

Okay, that's good to know.

Most other technologies won't have less dependencies.

Yeah, good point.

BTW, the tweaked jar only prevents a duplicate include of a service definiton that wouldn't cause harm, so a rather picky measurement.

I don't fully understand the benefit from preventing this duplicate. Can you explain that? To be honest, I'd feel much more comfortable with a build process that doesn't "tweak" provided library files. It feels fragile.

Replying to karsten:

... Let me suggest something totally different: Should we give up on JSPs now and migrate to facelets or whatever technology is recommended and supported these days?

Well, the embedded jetty is an improvement from a separate tomcat instance. The front-end can be tested locally now w/o much hassle and the control over the application is similar to what we have in CollecTor or Onionoo (and soon Metrics-Web).

In general, it is ok to look for other technology for the web part as a next step and viewed together with Metrics-Web and #23549 (moved).

Not sure if you saw my previous comment, as we were both commenting at the same time.

Just noticed comment:6

Replying to karsten:

... Different question then: if we need to include parts of embedded Tomcat, why don't we switch to that entirely? It's the embedded part that we're after, right? Or what else do we gain from picking Jetty in particular?

We've several embedded Jetties running and they work fine. Big projects like Jenkins/Hudson have been using embedded Jetty for years successfully. I didn't do research about the question which production system uses an embedded tomcat. Tomcat is more often used as separate instance or embedded into JBoss. By now Tomcat is almost an application server getting close to glassfish and the like. So, using the jsp functionality from Tomcat embeddable in an embedded Jetty is different from using embedded Tomcat. I find it a good sign that Jetty (Eclipse) uses working features others (Apache) built.

Here the package listing: libasm-java (5.2-2) libtomcat8-embed-java (8.5.14-1+deb9u2) libecj-java (3.11.1-1)

Okay, that's good to know.

Most other technologies won't have less dependencies.

Yeah, good point.

BTW, the tweaked jar only prevents a duplicate include of a service definiton that wouldn't cause harm, so a rather picky measurement.

I don't fully understand the benefit from preventing this duplicate. Can you explain that? To be honest, I'd feel much more comfortable with a build process that doesn't "tweak" provided library files. It feels fragile.

It only feels fragile on first sight: I prefer to use 'duplicate="fail"' setting for the war-task. The tweak only prevents the inclusion of a duplicate service definition. If you remove the 'duplicate="fail"' setting and the tweak-related lines all would still work as expected here. I'd rather include this for future dependency additions or upgrades to have an early warning, i.e., an immediate failure to assemble the war with new settings, when duplicate classes or configurations show up and then decide knowingly which should take precedence or what else to do.

Okay. Reviewing...

The dummy works okay, but I'm having trouble with the war file:

java.lang.reflect.InvocationTargetException: null
        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.eclipse.jetty.webapp.IterativeDescriptorProcessor.visit(IterativeDescriptorProcessor.java:85)
        at org.eclipse.jetty.webapp.IterativeDescriptorProcessor.process(IterativeDescriptorProcessor.java:72)
        at org.eclipse.jetty.webapp.MetaData.resolve(MetaData.java:408)
        at org.eclipse.jetty.webapp.WebAppContext.startContext(WebAppContext.java:1340)
        at org.eclipse.jetty.server.handler.ContextHandler.doStart(ContextHandler.java:741)
        at org.eclipse.jetty.webapp.WebAppContext.doStart(WebAppContext.java:505)
        at org.eclipse.jetty.util.component.AbstractLifeCycle.start(AbstractLifeCycle.java:68)
        at org.eclipse.jetty.util.component.ContainerLifeCycle.start(ContainerLifeCycle.java:132)
        at org.eclipse.jetty.util.component.ContainerLifeCycle.doStart(ContainerLifeCycle.java:114)
        at org.eclipse.jetty.server.handler.AbstractHandler.doStart(AbstractHandler.java:61)
        at org.eclipse.jetty.server.handler.ContextHandlerCollection.doStart(ContextHandlerCollection.java:163)
        at org.eclipse.jetty.util.component.AbstractLifeCycle.start(AbstractLifeCycle.java:68)
        at org.eclipse.jetty.util.component.ContainerLifeCycle.start(ContainerLifeCycle.java:132)
        at org.eclipse.jetty.server.Server.start(Server.java:387)
        at org.eclipse.jetty.util.component.ContainerLifeCycle.doStart(ContainerLifeCycle.java:114)
        at org.eclipse.jetty.server.handler.AbstractHandler.doStart(AbstractHandler.java:61)
        at org.eclipse.jetty.server.Server.doStart(Server.java:354)
        at org.eclipse.jetty.util.component.AbstractLifeCycle.start(AbstractLifeCycle.java:68)
        at org.torproject.exonerator.ServerMain.main(ServerMain.java:24)
Caused by: java.lang.IllegalStateException: Nothing to bind for name jdbc/exonerator
        at org.eclipse.jetty.plus.webapp.PlusDescriptorProcessor.bindEntry(PlusDescriptorProcessor.java:895)
        at org.eclipse.jetty.plus.webapp.PlusDescriptorProcessor.bindResourceRef(PlusDescriptorProcessor.java:812)
        at org.eclipse.jetty.plus.webapp.PlusDescriptorProcessor.visitResourceRef(PlusDescriptorProcessor.java:252)
        ... 23 common frames omitted

Did you get this working?

And where would I configure the JDBC connection string?

Replying to karsten:

The dummy works okay, ...

This is good!

but I'm having trouble with the war file: [snip] And where would I configure the JDBC connection string?

Argh, there is this open topic on my list here ... . Ok, setting to needs revision. But at least the tricky things work, sigh.

Thanks, for checking.

Trac:
Status: needs_review to needs_revision

So, now all is available for review on this branch. I ammended the initial commit with those changes I had forgotten, as it wouldn't help to review the difference. But I found some other things in addition, which have three more commits on their own.

For testing (and maybe other production server settings) we don't really want to query exonerator.tp.o. Thus, this commit makes it configurable.
The tests I did with the empty db also triggered an NPE and made me aware of connections that could stay unclosed. See this commit.
Database credentials are set in jetty.xml, but it might be useful to also configure these on the command line. This commit gives an example by using system properties.

Please review again.

Trac:
Status: needs_revision to needs_review

Looks good and works locally! Some comments/questions:

Can we use metrics and password as defaults in src/main/resources/jetty.xml? That should be as simple as writing <SystemProperty name="exonerator.db.user" default="metrics"/ > and <SystemProperty name="exonerator.db.pw" default="password"/ >. Just in case somebody uses those defaults locally for testing, that is.
Is there a particular reason why you suggest bumping to 2.0.0 and not 1.1.0? For the Java 8 update we also made a minor version bump. Nothing backward-incompatible going on here, right?
Out of curiousity, what's the full command line that you're using to start the JAR and the WAR file? Mine is pretty long, and I'm curious whether that's just for me or for everyone. Maybe we can start thinking about configuration libraries (#24041 (moved)) after this ticket?

I'll merge tomorrow after the first two questions are resolved. Setting to needs_revision only for these trivial tweaks.

And unless there are any reason not to deploy, I'd do that shortly after. Thanks!

Trac:
Status: needs_review to needs_revision

Replying to karsten:

Looks good and works locally!

Good, I assume all db access works fine now.

Some comments/questions:

Can we use metrics and password as defaults in src/main/resources/jetty.xml? That should be as simple as writing <SystemProperty name="exonerator.db.user" default="metrics"/ > and <SystemProperty name="exonerator.db.pw" default="password"/ >. Just in case somebody uses those defaults locally for testing, that is.

That means, you want your private test environment configuration in development code ;-)

I don't feel strongly about it. Add them, if you like. Just be aware that people looking at the code might think they discovered Tor's secret db credentials ;-) But, actually, this is part of a bigger discussion cf. my comment to #24041 (moved) about 'unifying deployment configuration'. Let's rather continue the configuration discussion there.

Is there a particular reason why you suggest bumping to 2.0.0 and not 1.1.0? For the Java 8 update we also made a minor version bump. Nothing backward-incompatible going on here, right?

Isn't it a major change to move away from external tomcat? The deployment changes completely.

Out of curiousity, what's the full command line that you're using to start the JAR and the WAR file? Mine is pretty long, and I'm curious whether that's just for me or for everyone. Maybe we can start thinking about configuration libraries (#24041 (moved)) after this ticket?

Deployment example: java -Dexonerator.db.user=secretuser -Dexonerator.db.pw=secret -jar exonerator-1.0.0-dev.war

Test w/o db: java -Dexonerator.url=http://127.0.0.1:8080 -jar exonerator-dummy-test.war

I did comment on #24041 (moved) and I'm not in favor of long command lines (they are better than hard-coded parameters), but that's a different ticket.

Replying to iwakeh:

Replying to karsten:

Looks good and works locally!

Good, I assume all db access works fine now.

Some comments/questions:

Can we use metrics and password as defaults in src/main/resources/jetty.xml? That should be as simple as writing <SystemProperty name="exonerator.db.user" default="metrics"/ > and <SystemProperty name="exonerator.db.pw" default="password"/ >. Just in case somebody uses those defaults locally for testing, that is.

That means, you want your private test environment configuration in development code ;-)

Asking for a friend! ;)

I don't feel strongly about it. Add them, if you like. Just be aware that people looking at the code might think they discovered Tor's secret db credentials ;-)

Well, without defaults, these are mandatory arguments, not options. I think it's fair to assume some default values, even if they are not what we'd use in a production environment.

But, actually, this is part of a bigger discussion cf. my comment to #24041 (moved) about 'unifying deployment configuration'. Let's rather continue the configuration discussion there.

Good idea!

Is there a particular reason why you suggest bumping to 2.0.0 and not 1.1.0? For the Java 8 update we also made a minor version bump. Nothing backward-incompatible going on here, right?

Isn't it a major change to move away from external tomcat? The deployment changes completely.

Alright. 2.0.0 it is.

Out of curiousity, what's the full command line that you're using to start the JAR and the WAR file? Mine is pretty long, and I'm curious whether that's just for me or for everyone. Maybe we can start thinking about configuration libraries (#24041 (moved)) after this ticket?

Deployment example: java -Dexonerator.db.user=secretuser -Dexonerator.db.pw=secret -jar exonerator-1.0.0-dev.war

Test w/o db: java -Dexonerator.url=http://127.0.0.1:8080 -jar exonerator-dummy-test.war

I did comment on #24041 (moved) and I'm not in favor of long command lines (they are better than hard-coded parameters), but that's a different ticket.

Thanks! Let's discuss this more on #24041 (moved).

Alright, I'll add defaults and merge either tonight or tomorrow.

Trac:
Status: needs_revision to merge_ready

Merged! Also deployed the .jar, but held back deploying the .war until tomorrow.

Deployed! Closing. Thanks again!

Trac:
Status: merge_ready to closed
Resolution: N/A to fixed

closed

mentioned in issue #24175 (moved)

Use an embedded Jetty in ExoneraTor

Child items ...

Activity