----------------------------------------------------------- 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 On 2010-07-26 18:52:01, Patrick Wendell wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://review.cloudera.org/r/390/ > ----------------------------------------------------------- > > (Updated 2010-07-26 18:52:01) > > > Review request for Avro. > > > Summary > ------- > > Here is a patch update based on the preliminary feedback. I dropped > the querying stuff since it's not yet used. This patch adds the logic > behind aggregating partial spans (which will have been collected from > several remote hosts) and creating traces (extracting RPC call trees > from a set of spans). > > We talked off-line about redundancy in the data that is collected. We > could get away with only recording a spanID and a parentSpanID for > each span, then we can back-out each trace by following these data (we > get to the root of a trace when we see a null parentSpanID). By adding > a traceID unique to each call tree, it is easier to partition the work > of collecting spans into traces as we can easily group spans by trace > before we try back-out a tree (perhaps this is why Dapper paper > describes this method). It also lowers the chance of ID collision > since spanID and parentSpanID are specific to one traceID. > > > Diffs > ----- > > lang/java/src/java/org/apache/avro/ipc/trace/InMemorySpanStorage.java > PRE-CREATION > lang/java/src/java/org/apache/avro/ipc/trace/SpanAggregator.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/Trace.java PRE-CREATION > lang/java/src/java/org/apache/avro/ipc/trace/TraceNode.java PRE-CREATION > lang/java/src/java/org/apache/avro/ipc/trace/TracePlugin.java PRE-CREATION > lang/java/src/java/org/apache/avro/ipc/trace/TracePluginConfiguration.java > PRE-CREATION > lang/java/src/test/java/org/apache/avro/ipc/trace/TestBasicTracing.java > PRE-CREATION > lang/java/src/test/java/org/apache/avro/ipc/trace/TestEndToEndTracing.java > PRE-CREATION > lang/java/src/test/java/org/apache/avro/ipc/trace/TestSpanAggregation.java > PRE-CREATION > > lang/java/src/test/java/org/apache/avro/ipc/trace/TestSpanTraceFormation.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.cloudera.org/r/390/diff > > > Testing > ------- > > Several unit-tests are present in this review. See testTraceAndCollection() > in TestEndToEndTracing.java for a clear example of the creation and > subsequent aggregation of trace data. > > > Thanks, > > Patrick > >
