----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/655/#review945 -----------------------------------------------------------
Cool stuff. Some comments below. lang/java/src/java/org/apache/avro/ipc/trace/StaticServlet.java <http://review.cloudera.org/r/655/#comment3103> This will throw an exception on an empty string. You should check for that and handle it more gracefully. Might be useful to write a quick test as well. lang/java/src/java/org/apache/avro/ipc/trace/TraceClientServlet.java <http://review.cloudera.org/r/655/#comment3104> This seems very ad hoc. What you're looking for is some sort of URL routing. You have a couple of options here: - Servlets have something called a RequestDispatcher which can be controlled by a "web.xml" file. - Or do it a bit more methodically, by extracting the path component out of the URL (instead of stripping http://, which is one-off, and may be broken because we might be using https), and then having some logic. It makes sense to put the logic for serving a specific view into its own function. If you had a more heavy-weight framework, it would certainly force you to do it. Either way, separating out the URL parsing logic from the collection logic seems like a nice thing to do. lang/java/src/java/org/apache/avro/ipc/trace/TraceCollection.java <http://review.cloudera.org/r/655/#comment3106> Are you not using primitive types so you can store 'null'? I think it would be better to use -1, but, either way, you should document it. lang/java/src/java/org/apache/avro/ipc/trace/TraceCollection.java <http://review.cloudera.org/r/655/#comment3105> If you have the getters, you may as well drop the 'public'. lang/java/src/java/org/apache/avro/ipc/trace/TraceCollection.java <http://review.cloudera.org/r/655/#comment3107> Commented out code? lang/java/src/java/org/apache/avro/ipc/trace/TraceCollection.java <http://review.cloudera.org/r/655/#comment3108> Indentation seems inconsistent here. lang/java/src/java/org/apache/avro/ipc/trace/TraceCollection.java <http://review.cloudera.org/r/655/#comment3109> More commented out code - Philip On 2010-08-16 16:10:06, Patrick Wendell wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://review.cloudera.org/r/655/ > ----------------------------------------------------------- > > (Updated 2010-08-16 16:10:06) > > > Review request for Avro and Philip Zeyliger. > > > Summary > ------- > > Basic collection of multiple traces and viewing servlet. Note: the javascript > files are external files, not mine. > > > Diffs > ----- > > lang/java/src/java/org/apache/avro/ipc/stats/StaticServlet.java 65a3cbe > lang/java/src/java/org/apache/avro/ipc/trace/SpanAggregator.java 65e4690 > lang/java/src/java/org/apache/avro/ipc/trace/StaticServlet.java > PRE-CREATION > lang/java/src/java/org/apache/avro/ipc/trace/Trace.java 46a2fce > lang/java/src/java/org/apache/avro/ipc/trace/TraceClientServlet.java > 016d4d4 > lang/java/src/java/org/apache/avro/ipc/trace/TraceCollection.java > PRE-CREATION > lang/java/src/java/org/apache/avro/ipc/trace/TraceNode.java 5d62aa2 > lang/java/src/java/org/apache/avro/ipc/trace/TracePlugin.java c63ca5b > lang/java/src/java/org/apache/avro/ipc/trace/TracePluginConfiguration.java > bc1aaee > lang/java/src/java/org/apache/avro/ipc/trace/Util.java 01c9e9e > lang/java/src/java/org/apache/avro/ipc/trace/static/g.bar.js PRE-CREATION > lang/java/src/java/org/apache/avro/ipc/trace/static/jquery.tipsy.js > PRE-CREATION > lang/java/src/java/org/apache/avro/ipc/trace/static/protovis-r3.2.js > PRE-CREATION > lang/java/src/java/org/apache/avro/ipc/trace/static/tipsy.js PRE-CREATION > lang/java/src/java/org/apache/avro/ipc/trace/templates/collection.vm > PRE-CREATION > lang/java/src/java/org/apache/avro/ipc/trace/templates/common.vm > PRE-CREATION > lang/java/src/java/org/apache/avro/ipc/trace/templates/overview.vm > PRE-CREATION > lang/java/src/java/org/apache/avro/ipc/trace/templates/traceinput.vm > PRE-CREATION > lang/java/src/test/java/org/apache/avro/ipc/trace/TestBasicTracing.java > 6bc9153 > lang/java/src/test/java/org/apache/avro/ipc/trace/TestTraceCollection.java > PRE-CREATION > share/schemas/org/apache/avro/ipc/trace/avroTrace.avpr 6cf25d2 > > Diff: http://review.cloudera.org/r/655/diff > > > Testing > ------- > > TestTraceCollection.java tests the new TraceCollection class. > > > Thanks, > > Patrick > >
