----------------------------------------------------------- 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. 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. 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. - 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 > >
