[AVRO-1213] Update to latest release of Jetty #244
Conversation
|
Looks good to me. Unit tests pass with this change on top of the current master. |
commanderofthegrey
left a comment
commanderofthegrey
left a comment
There was a problem hiding this comment.
Hi @dkulp ,
Thanks for the changes, this has been a pain it seems for a while, I have a few questions/comments.
Please review and reply to them.
Also, did you test this with any user use-case or only the unit tests? As I've been looking for a use-case for this and it would be helpful if you had one.
Thanks,
/cmd
| @@ -48,14 +38,14 @@ public Server createServer(Responder testResponder) throws Exception { | |||
| System.setProperty("javax.net.ssl.password", "avrotest"); | |||
| System.setProperty("javax.net.ssl.trustStore", "src/test/truststore"); | |||
| System.setProperty("javax.net.ssl.trustStorePassword", "avrotest"); | |||
There was a problem hiding this comment.
It might make sense to set the rest of the properties on the connector which were declared above, or remove those if they are not used/needed.
There was a problem hiding this comment.
I believe those system properties are also used by the client side portions (which I haven't touched) to setup the SSL stuff for the client that connect to it.
|
|
||
| ServletHandler handler = new ServletHandler(); | ||
| server.setHandler(handler); | ||
| handler.addServletWithMapping(new ServletHolder(servlet), "/*"); |
There was a problem hiding this comment.
I might be off here, but wouldn't using ServerContextHandler be more appropriate here?
| this.server = new org.mortbay.jetty.Server(); | ||
| SelectChannelConnector connector = new SelectChannelConnector(); | ||
| connector.setLowResourceMaxIdleTime(10000); | ||
| connector.setAcceptQueueSize(128); |
There was a problem hiding this comment.
Would it make sense to set this, as well as setIdleTime so it's more similar to the previous version?
| + " \"request\": [{\"name\": \"millis\", \"type\": \"long\"}," + | ||
| "{\"name\": \"data\", \"type\": \"bytes\"}], " | ||
| + " \"response\": \"null\"} } }"); | ||
| Log.info("Using protocol: " + protocol.toString()); |
There was a problem hiding this comment.
While it probably makes sense to remove the log, as that way we don't have to add jetty-util as a dependency, but would it not help if we made it more of an exact match initially and later decide whether this Log message even matters?
| } | ||
|
|
||
| /** Constructs a server to run with the given connector. */ | ||
| public HttpServer(Responder responder, Connector connector) throws IOException { |
There was a problem hiding this comment.
Is the diff wonky or was the method with the "public HttpServer(ResponderServlet servlet, Connector connector) throws IOException" signature removed? If they were removed in my opinion it would hurt backwards compatibility in this case.
|
I went ahead and rebased it on the latest code on master so it's a clean "merge". I also fixed/added the timeout and accept queue length. I kind of went back and forth on that. The Jetty folks have done a bunch of work to fine tune things internally since jetty 7 so the defaults are usually "sane". However, setting those is fine and doesn't really hurt anything. I also went ahead and used the ServletContextHandler instead of a ServletHandler directly. The ServletContextHandler does provide some extra stuff (like compression support and security and things), but we don't use any of that so it's not really necessary. Having it in place though might open up the opportunity for those in the future. Regarding the API change (constructor args), it's more or less mandatory or there is NO way to upgrade to a newer Jetty. The problem is that it takes a Jetty object (org.mortbay.jetty.Connector) which no longer exists. It's nearest similar class is ConnectionFactory, but that's in the new org.eclipse.jetty namespace. Thus, the API change has to happen or it's stuck on Jetty 7. It's a bit unfortunate that an implementation detail (jetty usage) was exposed this way. I supposed I could change the constructor to just take "Object" and cast it internally and throw an exception if it's not something we can use, but that just moves it from a compile issue to a runtime issue which sucks. |
|
Thanks for the changes @dkulp ! You are right about the API changing anyways, I was thinking more along the lines that it might be less disruptive if we provided a similar constructor, just with org.eclipse.jetty.server.Connector instead of the mortbay one, but not sure if this would be considered helpful or not. Best, |
|
@dkulp how about leaving both constructors, adding the two new constructor you proposed, and marking the original two as deprecated? You're right, this is an incompatible change anyway, but in my opinion, changing the package name is a simpler task at the client side than understanding why the method has two additional parameter and why did the type of the second parameter changed. What do you think? |
|
I went ahead an added the constructor, but I'm not really sure how "useful" it is as it isn't "just a package name change" to use it. The old jetty, you could "new" a Connector, configure it, and pass it in and we'd attach it to the "Server" we created. That doesn't work in the new jetty as the Connector has to have the server as a constructor arg. Thus, the user needs to know to not just change the package, but also create a new server and pass that in. Anyway, not a huge deal to add the constructor so I did. |
|
@dkulp could you please add the other constructor ( |
Closes apache#244 Signed-off-by: sacharya <suraj@apache.org> Signed-off-by: Nandor Kollar <nkollar@apache.org> Signed-off-by: Anna Szonyi <szonyi@cloudera.com>
5 participants
This updates the Jetty dependency to the latest Jetty release.
Note: this does not address the Jetty vs. Netty thing or updates to the latest Netty. I hope to tackle that soon, but the netty update is huge/hard with a much larger impact.
The API signatures do change slightly, but that is obviously required due to the org.mortbay -> org.eclipse change. However, getting onto the supported version of Jetty (with the latest security updates and fixes) is important.