[jira] Commented: (AVRO-613) Create basic frontend to view trace results.

["Philip Zeyliger (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/AVRO-613?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12899995#action_12899995 ] 

Philip Zeyliger commented on AVRO-613:
--------------------------------------

Made some comments via reviewboard:


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


> Create basic frontend to view trace results.
> --------------------------------------------
>
>                 Key: AVRO-613
>                 URL: https://issues.apache.org/jira/browse/AVRO-613
>             Project: Avro
>          Issue Type: Sub-task
>            Reporter: Patrick Wendell
>            Assignee: Patrick Wendell
>         Attachments: AVRO-613.v1.txt, AVRO-613.v2.txt
>
>


-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.