[
https://issues.apache.org/jira/browse/AVRO-595?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12890911#action_12890911
]
Philip Zeyliger commented on AVRO-595:
--------------------------------------
I put up a review on reviewboard:
-----------------------------------------------------------
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)
- Show quoted text -
> Add Dapper-Style Tracing Plugin to Avro
> ---------------------------------------
>
> Key: AVRO-595
> URL: https://issues.apache.org/jira/browse/AVRO-595
> Project: Avro
> Issue Type: Sub-task
> Reporter: Patrick Wendell
> Assignee: Patrick Wendell
> Attachments: AVRO-595.patch.v1.txt
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
