AVRO-587

Review Request #243 - Created June 30, 2010 and updated

Philip Zeyliger
old-avro git
avro
This is https://issues.apache.org/jira/secure/attachment/12448426/AVRO-587.patch.v2.txt


  1. 
      
  2. (style nit) Non-local variables should probably have longer names.  vEngine or velocityEngine is more typical.
  3. Should the two ve.addProperty lines both be there?  Maybe put them underneath the same comment or explain the difference.
  4. Javadoc?  Does this need to be public?
  5. Seems like this wouldn't handle the string 'foo"bar' correctly, since the quotes themselves would need escaping.
  6. One alternative here is to use a directory ("/static/") to determine this, rather than the extension.
  7. lang/java/src/java/org/apache/avro/ipc/stats/StatsServlet.java (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
     
     
     
    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.
  8. Does keySet() make a copy?  If it doesn't (and I kind of doubt it), the synchronized isn't really helping you.
  9. This looks like it could be a private static final field of the class; no need to create one of these at every request.
  10. Any reason not to put this in the template itself?  Seems weird to have both templates and string concatenation.
  11. Could this be in the temlate?
  12. 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?
  13. If velocity templates have comments, would be good to put a license file up there.
  14. Maybe it's appropriate to pull this script out into a separate file?
  15. 
      
Loading...