[
https://issues.apache.org/jira/browse/AVRO-587?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12884041#action_12884041
]
Philip Zeyliger commented on AVRO-587:
--------------------------------------
Patrick,
Good work getting this working!
I made some comments on the reviewboard copy of this. The things i'm concerned
about are the handling of static resources (seems better to leverage an
existing servlet) and whether or not you could move even more into the
templates.
-- Philip
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?
> Add Charts and Templating to Stats View
> ---------------------------------------
>
> Key: AVRO-587
> URL: https://issues.apache.org/jira/browse/AVRO-587
> Project: Avro
> Issue Type: Sub-task
> Reporter: Patrick Wendell
> Assignee: Patrick Wendell
> Attachments: AVRO-587.patch.v1.txt, AVRO-587.patch.v2.txt
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
