ReviewBoard is awesome. I'll need to figure out how to use it sometime. Comments inline: On Jul 7, 2010, at 4:51 PM, Philip Zeyliger wrote:
> > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://review.hbase.org/r/281/#review320 > ----------------------------------------------------------- > > > The code, besides the catch (Exception e), looks fine. > > I do worry that this is a massively backwards-incompatible change. Anyone > who's set up an Avro RPC server using HttpServer now needs to add a line to > their code. I can only think of hacky ways around that. > Yeah, that is a concern. This is definitely an improvement though. > > trunk/lang/java/src/java/org/apache/avro/ipc/HttpServer.java > <http://review.hbase.org/r/281/#comment1391> > > I would add to the java doc that this throws AvroRuntimeException. > "@throws AvroRuntimeException if the underlying Jetty server throws any exception while starting." ? > > > trunk/lang/java/src/java/org/apache/avro/ipc/HttpServer.java > <http://review.hbase.org/r/281/#comment1392> > > catch (Exception e) is poor form, typically. It's usually better to > explicitly catch the type of exception that Jetty's start throws. > True, but Jetty's start() signature is: void org.mortbay.component.AbstractLifeCycle.start() throws Exception So my only caution with this patch is the backwards-incompatible change. Luckily it is not subtle. I'm fine with the change but am not a major user of HttpServer. This does likely imply a change for other Servers down the road too. > > - Philip > > > On 2010-07-07 16:42:45, Jeff Hammerbacher wrote: >> >> ----------------------------------------------------------- >> This is an automatically generated e-mail. To reply, visit: >> http://review.hbase.org/r/281/ >> ----------------------------------------------------------- >> >> (Updated 2010-07-07 16:42:45) >> >> >> Review request for Avro. >> >> >> Summary >> ------- >> >> Adding start() to the Server interface and start() and join() to the >> HttpServer implementation. Moved start() out of the HttpServer ctor and >> changed all places it was called in the codebase to now grab an object and >> call start() on it. >> >> >> This addresses bug AVRO-544. >> http://issues.apache.org/jira/browse/AVRO-544 >> >> >> Diffs >> ----- >> >> trunk/lang/java/src/java/org/apache/avro/ipc/HttpServer.java 960285 >> trunk/lang/java/src/java/org/apache/avro/ipc/Server.java 960285 >> trunk/lang/java/src/java/org/apache/avro/tool/RpcReceiveTool.java 960285 >> trunk/lang/java/src/test/java/org/apache/avro/TestBulkData.java 960285 >> trunk/lang/java/src/test/java/org/apache/avro/TestProtocolHttp.java 960285 >> >> trunk/lang/java/src/test/java/org/apache/avro/ipc/stats/StatsPluginOverhead.java >> 960285 >> >> trunk/lang/java/src/test/java/org/apache/avro/ipc/stats/TestStatsPluginAndServlet.java >> 960285 >> >> Diff: http://review.hbase.org/r/281/diff >> >> >> Testing >> ------- >> >> ant test-java >> >> >> Thanks, >> >> Jeff >> >> >
