[prev in list] [next in list] [prev in thread] [next in thread] 

List:       avro-dev
Subject:    [jira] Commented: (AVRO-587) Add Charts and Templating to Stats View
From:       "Philip Zeyliger (JIRA)" <jira () apache ! org>
Date:       2010-06-30 20:01:50
Message-ID: 14067826.140461277928110383.JavaMail.jira () thor
[Download RAW message or body]


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


[prev in list] [next in list] [prev in thread] [next in thread]