[
https://issues.apache.org/jira/browse/AVRO-595?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12893374#action_12893374
]
Philip Zeyliger commented on AVRO-595:
--------------------------------------
Another review via reviewboard:
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/390/#review521
-----------------------------------------------------------
Good stuff, Patrick. A few concerns about InMemorySpanStorage and a few other
things in the review below.
lang/java/src/java/org/apache/avro/ipc/trace/InMemorySpanStorage.java
<http://review.cloudera.org/r/390/#comment2088>
nitpick: grammar is wonky here.
lang/java/src/java/org/apache/avro/ipc/trace/InMemorySpanStorage.java
<http://review.cloudera.org/r/390/#comment2089>
Seems like you probably have concurrency issues here: a multi-threaded
server shares one span storage, so, unless there's a synchronized somewhere
above this, this doesn't work.
lang/java/src/java/org/apache/avro/ipc/trace/InMemorySpanStorage.java
<http://review.cloudera.org/r/390/#comment2091>
This is tricky to use on a running server because the spans are still in
play. You could make a copy, which is expensive, but since real clients would
use files...
lang/java/src/java/org/apache/avro/ipc/trace/SpanAggregator.java
<http://review.cloudera.org/r/390/#comment2102>
I suspect that SpanEvent.values() is what you're looking for here, instead
of enumerating all the enums.
lang/java/src/java/org/apache/avro/ipc/trace/SpanAggregator.java
<http://review.cloudera.org/r/390/#comment2109>
You do these 5 lines twice; may as well make a helper function.
lang/java/src/java/org/apache/avro/ipc/trace/SpanAggregator.java
<http://review.cloudera.org/r/390/#comment2110>
After reading this code a couple of times, I think it may be simpler if you
follow more of a map-reduce pattern.
First group all the spans. Then go through every grouping and see if it's
complete. Your code keeps checking to see if the grouping is complete as you
go, which makes the logic a little harder to follow.
Just a suggestion; it works either way.
lang/java/src/java/org/apache/avro/ipc/trace/SpanAggregator.java
<http://review.cloudera.org/r/390/#comment2107>
Look into using EnumSet here. It's probably more efficient than a
linkedlist.
lang/java/src/java/org/apache/avro/ipc/trace/SpanAggregator.java
<http://review.cloudera.org/r/390/#comment2108>
is this 'if' ever false if you have a complete span between the two?
lang/java/src/java/org/apache/avro/ipc/trace/SpanAggregator.java
<http://review.cloudera.org/r/390/#comment2111>
Since traces.get(traceID) is the next thing you do, you can probably iterate
over the items in traces to begin with.
lang/java/src/java/org/apache/avro/ipc/trace/Trace.java
<http://review.cloudera.org/r/390/#comment2112>
Do you actually need the empty constructor and the setter? If you can avoid
two ways of doing something, that's often better. You might need it, of course.
lang/java/src/java/org/apache/avro/ipc/trace/Trace.java
<http://review.cloudera.org/r/390/#comment2113>
You might mention that the graphi is meant to be read with your head askew.
I found the encoding a bit hard to read. Perhaps (w (x) (y (z)) (more of an
S-expression-style tree) would be easier to read? Your call.
lang/java/src/java/org/apache/avro/ipc/trace/Trace.java
<http://review.cloudera.org/r/390/#comment2114>
Extract 1000000 to a constant. (MICROS_PER_SECOND?)
lang/java/src/java/org/apache/avro/ipc/trace/Trace.java
<http://review.cloudera.org/r/390/#comment2115>
I think our style guide typically recommends
if (...) {
...
} else {
...
}
(i.e., put else on the same line as the two braces)
lang/java/src/java/org/apache/avro/ipc/trace/Trace.java
<http://review.cloudera.org/r/390/#comment2116>
Naively, if you structure the getChildren() function nicely, you could get
away without managing the root separately, since every subnode is it's own
"root". Not sure if that's easy to do in this case without trying it.
lang/java/src/java/org/apache/avro/ipc/trace/TracePlugin.java
<http://review.cloudera.org/r/390/#comment2117>
I think you've just re-implemented Arrays.equals(byte[] a, byte[] b).
- Philip
> 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, AVRO-595.patch.v2.txt
>
>
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
