You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by Philip Zeyliger <ph...@cloudera.com> on 2010/06/30 21:35:13 UTC
Review Request: AVRO-587
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/243/
-----------------------------------------------------------
Review request for Avro.
Summary
-------
This is https://issues.apache.org/jira/secure/attachment/12448426/AVRO-587.patch.v2.txt
Diffs
-----
lang/java/ivy.xml ba4e7d6
lang/java/src/java/org/apache/avro/ipc/stats/StatsServlet.java 642e374
lang/java/src/java/org/apache/avro/ipc/stats/templates/g.bar.js PRE-CREATION
lang/java/src/java/org/apache/avro/ipc/stats/templates/jquery-1.4.2.min.js PRE-CREATION
lang/java/src/java/org/apache/avro/ipc/stats/templates/jquery.tipsy.js PRE-CREATION
lang/java/src/java/org/apache/avro/ipc/stats/templates/protovis-r3.2.js PRE-CREATION
lang/java/src/java/org/apache/avro/ipc/stats/templates/statsview.vm PRE-CREATION
lang/java/src/java/org/apache/avro/ipc/stats/templates/tipsy.css PRE-CREATION
lang/java/src/java/org/apache/avro/ipc/stats/templates/tipsy.js PRE-CREATION
lang/java/src/test/java/org/apache/avro/ipc/stats/TestStatsPluginAndServlet.java d954a6e
Diff: http://review.hbase.org/r/243/diff
Testing
-------
Thanks,
Philip
Re: Review Request: AVRO-587
Posted by Philip Zeyliger <ph...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/243/#review294
-----------------------------------------------------------
lang/java/src/java/org/apache/avro/ipc/stats/StatsServlet.java
<http://review.hbase.org/r/243/#comment1316>
(style nit) Non-local variables should probably have longer names. vEngine or velocityEngine is more typical.
lang/java/src/java/org/apache/avro/ipc/stats/StatsServlet.java
<http://review.hbase.org/r/243/#comment1317>
Should the two ve.addProperty lines both be there? Maybe put them underneath the same comment or explain the difference.
lang/java/src/java/org/apache/avro/ipc/stats/StatsServlet.java
<http://review.hbase.org/r/243/#comment1318>
Javadoc? Does this need to be public?
lang/java/src/java/org/apache/avro/ipc/stats/StatsServlet.java
<http://review.hbase.org/r/243/#comment1319>
Seems like this wouldn't handle the string 'foo"bar' correctly, since the quotes themselves would need escaping.
lang/java/src/java/org/apache/avro/ipc/stats/StatsServlet.java
<http://review.hbase.org/r/243/#comment1320>
One alternative here is to use a directory ("/static/") to determine this, rather than the extension.
lang/java/src/java/org/apache/avro/ipc/stats/StatsServlet.java
<http://review.hbase.org/r/243/#comment1321>
A couple of problems here:
(1) mediaFileName might be "../../../" which might let folks read arbitrary data from the classpath: not a great idea. (I actually think I've had trouble using .. in resources before, but that doesn't mean that it's not worrisome).
(2) Indentation is wonky.
(3) Using is.read() to get a single byte is slower than reading buffers at a time. Look around for utility methods to read fully from a stream into another stream. I know Hadoop has some; not sure about Avro's code base.
Ultimately, I think it might be wiser to delegate to an existing servlet for serving static files. Something like http://jetty.codehaus.org/jetty/jetty-6/apidocs/org/mortbay/jetty/servlet/DefaultServlet.html . If the request matches a certain path, just have that servlet do it. That servlet is more likely to do cache headers, mime types, and efficient IO than you are.
You might have to do some hunting to find the resources. Looks like that might be done by subclassing and over-riding getResource().
(4) I would separate the static and dynamic code paths into two methods.
lang/java/src/java/org/apache/avro/ipc/stats/StatsServlet.java
<http://review.hbase.org/r/243/#comment1322>
Does keySet() make a copy? If it doesn't (and I kind of doubt it), the synchronized isn't really helping you.
lang/java/src/java/org/apache/avro/ipc/stats/StatsServlet.java
<http://review.hbase.org/r/243/#comment1323>
This looks like it could be a private static final field of the class; no need to create one of these at every request.
lang/java/src/java/org/apache/avro/ipc/stats/StatsServlet.java
<http://review.hbase.org/r/243/#comment1324>
Any reason not to put this in the template itself? Seems weird to have both templates and string concatenation.
lang/java/src/java/org/apache/avro/ipc/stats/StatsServlet.java
<http://review.hbase.org/r/243/#comment1325>
Could this be in the temlate?
lang/java/src/java/org/apache/avro/ipc/stats/StatsServlet.java
<http://review.hbase.org/r/243/#comment1326>
weird indent
lang/java/src/java/org/apache/avro/ipc/stats/templates/g.bar.js
<http://review.hbase.org/r/243/#comment1327>
Apache projects have a "rat" tool that checks that licenses are ok. Could you make sure that when "rat' is run with this file included, it's happy with it?
lang/java/src/java/org/apache/avro/ipc/stats/templates/statsview.vm
<http://review.hbase.org/r/243/#comment1329>
If velocity templates have comments, would be good to put a license file up there.
lang/java/src/java/org/apache/avro/ipc/stats/templates/statsview.vm
<http://review.hbase.org/r/243/#comment1328>
Maybe it's appropriate to pull this script out into a separate file?
- Philip
On 2010-06-30 12:35:12, Philip Zeyliger wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.hbase.org/r/243/
> -----------------------------------------------------------
>
> (Updated 2010-06-30 12:35:12)
>
>
> Review request for Avro.
>
>
> Summary
> -------
>
> This is https://issues.apache.org/jira/secure/attachment/12448426/AVRO-587.patch.v2.txt
>
>
> Diffs
> -----
>
> lang/java/ivy.xml ba4e7d6
> lang/java/src/java/org/apache/avro/ipc/stats/StatsServlet.java 642e374
> lang/java/src/java/org/apache/avro/ipc/stats/templates/g.bar.js PRE-CREATION
> lang/java/src/java/org/apache/avro/ipc/stats/templates/jquery-1.4.2.min.js PRE-CREATION
> lang/java/src/java/org/apache/avro/ipc/stats/templates/jquery.tipsy.js PRE-CREATION
> lang/java/src/java/org/apache/avro/ipc/stats/templates/protovis-r3.2.js PRE-CREATION
> lang/java/src/java/org/apache/avro/ipc/stats/templates/statsview.vm PRE-CREATION
> lang/java/src/java/org/apache/avro/ipc/stats/templates/tipsy.css PRE-CREATION
> lang/java/src/java/org/apache/avro/ipc/stats/templates/tipsy.js PRE-CREATION
> lang/java/src/test/java/org/apache/avro/ipc/stats/TestStatsPluginAndServlet.java d954a6e
>
> Diff: http://review.hbase.org/r/243/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Philip
>
>