avro-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Philip Zeyliger" <phi...@cloudera.com>
Subject Re: Review Request: AVRO-595
Date Wed, 21 Jul 2010 21:38:28 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/351/#review443
-----------------------------------------------------------


Great start.  Comments below.


lang/java/src/java/org/apache/avro/ipc/trace/InMemorySpanStorage.java
<http://review.hbase.org/r/351/#comment1866>

    queryHandle seems unused here.  The query stuff still seems to be baking; I haven't seen
if your tests depend on it, but it might make sense to stage it out of this commit and put
it back in later when it's more strictly necessary.



lang/java/src/java/org/apache/avro/ipc/trace/SpanStorage.java
<http://review.hbase.org/r/351/#comment1864>

    Drop @author's for Apache code



lang/java/src/java/org/apache/avro/ipc/trace/SpanStorage.java
<http://review.hbase.org/r/351/#comment1865>

    It's weird that this returns bytes and not objects...



lang/java/src/java/org/apache/avro/ipc/trace/TracePlugin.java
<http://review.hbase.org/r/351/#comment1851>

    mention how this might be disabled?



lang/java/src/java/org/apache/avro/ipc/trace/TracePlugin.java
<http://review.hbase.org/r/351/#comment1852>

    Could you initialize these guys in-line and mark them final as well?



lang/java/src/java/org/apache/avro/ipc/trace/TracePlugin.java
<http://review.hbase.org/r/351/#comment1853>

    I'm not a big fan of using Configuration here.  Even though AVRO has a dependency on Hadoop,
it seems a bit abusive to use it where there's no Hadoop going on.
    
    I recommend a POJO represingting TracePluginConfiguration which people can create and
pass to this.
    
    If you were using Hadoop configuration, you'd want the keys to be better namespaced (i.e.,
avro.trace.foo instead of just foo), since people would want to use the same global configuration
mechanism.  But my opinion is that the layer that's doing the work should use reasonably typed
configuration, and if you want to layer something above it, that can be done above.



lang/java/src/java/org/apache/avro/ipc/trace/TracePlugin.java
<http://review.hbase.org/r/351/#comment1854>

    These are static, so you shouldn't need to instantiate them per instance.



lang/java/src/java/org/apache/avro/ipc/trace/TracePlugin.java
<http://review.hbase.org/r/351/#comment1855>

    Why Math.abs()?  Is spanID a fixed(4) or a long?  It'll store more efficiently if it's
a fixed(4), though it might be more of a pain to use.



lang/java/src/java/org/apache/avro/ipc/trace/TracePlugin.java
<http://review.hbase.org/r/351/#comment1856>

    What's the 100 doing here?



lang/java/src/java/org/apache/avro/ipc/trace/TracePlugin.java
<http://review.hbase.org/r/351/#comment1857>

    Cache this?



lang/java/src/java/org/apache/avro/ipc/trace/TracePlugin.java
<http://review.hbase.org/r/351/#comment1858>

    Extract "createNewEmptySpan()" into a method; you've done the same thing twice.



lang/java/src/java/org/apache/avro/ipc/trace/TracePlugin.java
<http://review.hbase.org/r/351/#comment1859>

    You should document (perhaps in package.html, or in javadoc here) how this system uses
the RPC metadata.



lang/java/src/java/org/apache/avro/ipc/trace/TracePlugin.java
<http://review.hbase.org/r/351/#comment1860>

    Since these aren't supposed to happen, I tend to escalate them into RuntimeException instead
of dropping them on the floor.  Just in case :)



lang/java/src/java/org/apache/avro/ipc/trace/TracePlugin.java
<http://review.hbase.org/r/351/#comment1861>

    I'm a little confused as to why you need both span and parent_span, but I might be missing
something.



lang/java/src/java/org/apache/avro/ipc/trace/TracePlugin.java
<http://review.hbase.org/r/351/#comment1862>

    Log about this; you'd want to know that something is misbehaving.



lang/java/src/java/org/apache/avro/ipc/trace/TracePlugin.java
<http://review.hbase.org/r/351/#comment1863>

    You're using childSpan and this.childSpan inconsistency in a few places; may as well make
it consistent.



lang/java/src/test/java/org/apache/avro/ipc/trace/TestBasicTracing.java
<http://review.hbase.org/r/351/#comment1868>

    Looks like x, y, w here.



lang/java/src/test/java/org/apache/avro/ipc/trace/TestBasicTracing.java
<http://review.hbase.org/r/351/#comment1869>

    I wonder if this test would read easier if you used the specific API and generated some
classes for it.  Not a big concern.



share/schemas/org/apache/avro/ipc/trace/avroTrace.avdl
<http://review.hbase.org/r/351/#comment1870>

    Perhaps add a microseconds component to this as well?



share/schemas/org/apache/avro/ipc/trace/avroTrace.avdl
<http://review.hbase.org/r/351/#comment1847>

    It's a little weird that "SpanEventType" isn't really a type; it's the event itself.
    
    Perhaps, rename SpanEventType as SpanEvent?



share/schemas/org/apache/avro/ipc/trace/avroTrace.avdl
<http://review.hbase.org/r/351/#comment1848>

    Document each of these fields?



share/schemas/org/apache/avro/ipc/trace/avroTrace.avdl
<http://review.hbase.org/r/351/#comment1849>

    It might be useful to have both host and port.



share/schemas/org/apache/avro/ipc/trace/avroTrace.avpr
<http://review.hbase.org/r/351/#comment1850>

    Is this generated?  Could this be generated by the build instead of checked in?


- Philip


On 2010-07-21 13:02:51, Philip Zeyliger wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.hbase.org/r/351/
> -----------------------------------------------------------
> 
> (Updated 2010-07-21 13:02:51)
> 
> 
> Review request for Avro.
> 
> 
> Summary
> -------
> 
> AVRO-595
> 
> 
> Diffs
> -----
> 
>   lang/java/ivy.xml a781557 
>   lang/java/src/java/org/apache/avro/ipc/trace/InMemorySpanStorage.java PRE-CREATION

>   lang/java/src/java/org/apache/avro/ipc/trace/SpanStorage.java PRE-CREATION 
>   lang/java/src/java/org/apache/avro/ipc/trace/TracePlugin.java PRE-CREATION 
>   lang/java/src/test/java/org/apache/avro/ipc/trace/TestBasicTracing.java PRE-CREATION

>   share/schemas/org/apache/avro/ipc/trace/avroTrace.avdl PRE-CREATION 
>   share/schemas/org/apache/avro/ipc/trace/avroTrace.avpr PRE-CREATION 
> 
> Diff: http://review.hbase.org/r/351/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Philip
> 
>


Mime
View raw message