avro-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Patrick Wendell" <pwend...@cloudera.com>
Subject Re: Review Request: AVRO-613
Date Wed, 18 Aug 2010 22:46:49 GMT


> On 2010-08-18 12:50:51, Philip Zeyliger wrote:
> > lang/java/src/java/org/apache/avro/ipc/trace/TraceClientServlet.java, line 170
> > <http://review.cloudera.org/r/655/diff/2/?file=7322#file7322line170>
> >
> >     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.

Yep, agree it makes sense do modularize the view functions. I will probably opt for method
(2) because tooling around with the Servlet stuff always confuses me. If we end up expanding
to a ton of views we can always plug that in afterwards.


- Patrick


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/655/#review945
-----------------------------------------------------------


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


Mime
View raw message