You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by Patrick Wendell <pw...@cloudera.com> on 2010/08/14 01:34:53 UTC
Review Request: AVRO-616
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/655/
-----------------------------------------------------------
Review request for Avro.
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
Re: Review Request: AVRO-613
Posted by Patrick Wendell <pw...@cloudera.com>.
> 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
>
>
Re: Review Request: AVRO-613
Posted by Philip Zeyliger <ph...@cloudera.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request: AVRO-613
Posted by Patrick Wendell <pw...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/655/
-----------------------------------------------------------
(Updated 2010-08-19 17:13:43.057382)
Review request for Avro and Philip Zeyliger.
Changes
-------
Another rev of this patch addressing the comments in this review.
Summary
-------
Basic collection of multiple traces and viewing servlet. Note: the javascript files are external files, not mine.
Diffs (updated)
-----
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/avrotrace.js PRE-CREATION
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/node.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/TestStaticServlet.java PRE-CREATION
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
Re: Review Request: AVRO-613
Posted by Patrick Wendell <pw...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/655/
-----------------------------------------------------------
(Updated 2010-08-16 16:10:06.234676)
Review request for Avro and Philip Zeyliger.
Changes
-------
Did some re-factoring on this today so uploading the new diff.
Summary
-------
Basic collection of multiple traces and viewing servlet. Note: the javascript files are external files, not mine.
Diffs (updated)
-----
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
Re: Review Request: AVRO-613
Posted by Patrick Wendell <pw...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/655/
-----------------------------------------------------------
(Updated 2010-08-16 11:48:59.102948)
Review request for Avro and Philip Zeyliger.
Changes
-------
Changing name to correctly match the JIRA.
Summary (updated)
-------
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