[jira] Commented: (AVRO-606) Add File-Based Span Storage to TracePlugin

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

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

Philip Zeyliger commented on AVRO-606:
--------------------------------------

bq. simply drop oldest file if

Nit: missing "the", and perhaps insert a "we" in there, to indicate that this is what the code does.

bq. private Long currentTimestamp

Why not "long"?

bq.  private static int SECONDS_PER_FILE = 60 * 10; // ten minute chunks

Consider making this configurable.

bq. catch (InterruptedException e1) {

I think (http://www.ibm.com/developerworks/java/library/j-jtp05236.html) you're supposed to call Thread.currentThread().interrupt(); at this point.  I kind of hate this part of Java.

You could also allow yourself to get interrupted, as a flow control mechanism.

Look around to see what other Hadoop worker threads do.

bq. } catch (IOException e) {

You should probably log the exception.

You should also probably catch all Throwables, and log those too.  If you don't, your thread tends to die, and nobody tends to know where it went.

bq.  this.spansSoFar -= spansPerFile.get(oldest);

Is this always the same as this.spansSoFar = 0?

bq. if (createNewFile) {

Wouldn't you want to set spansSoFar to 0 if you're writing a new file?

bq. +  private static String TRACE_FILE_DIR = "/tmp";

This stuff ought to be configurable.

bq. Thread writer;

Why not private?

bq. +  Boolean writerEnabled;

Why not primitive type?

bq. private static void addFileSpans

this feels like "readFileSpans()" more so than "add".  

bq. private static void addFileSpans

You could do binary search here, but for now it's probably fine to just indicate that it's linear.

bq. public static void setFileGranularityForTesting(int granularity) {

Package-private (default) is probably good enough here.

bq. protected void finalize() {

Finalize is almost always a bad idea; why is this here?

bq. // TODO Auto-generated method stub

Throw UnsupportedOperationException() here.

bq. * @param start UNIX time (in nanoseconds) as a long

Huh?  I thought this was millis/1000, i.e., seconds and not nanos.

> Add File-Based Span Storage to TracePlugin
> ------------------------------------------
>
>                 Key: AVRO-606
>                 URL: https://issues.apache.org/jira/browse/AVRO-606
>             Project: Avro
>          Issue Type: Sub-task
>            Reporter: Patrick Wendell
>            Assignee: Patrick Wendell
>         Attachments: AVRO-606.v1.txt, AVRO-606.v2.txt
>
>


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