> On 2010-07-28 14:25:15, Philip Zeyliger wrote: > > Good stuff, Patrick. A few concerns about InMemorySpanStorage and a few > > other things in the review below.
Addressed all of these comments - thanks for the review! > On 2010-07-28 14:25:15, Philip Zeyliger wrote: > > lang/java/src/java/org/apache/avro/ipc/trace/InMemorySpanStorage.java, line > > 35 > > <http://review.cloudera.org/r/390/diff/1/?file=3379#file3379line35> > > > > 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. Yes - ran into this problem after putting up this code! Now includes synchronization on the span array. > On 2010-07-28 14:25:15, Philip Zeyliger wrote: > > lang/java/src/java/org/apache/avro/ipc/trace/Trace.java, lines 216-221 > > <http://review.cloudera.org/r/390/diff/1/?file=3382#file3382line216> > > > > 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. Yes, changed this. > On 2010-07-28 14:25:15, Philip Zeyliger wrote: > > lang/java/src/java/org/apache/avro/ipc/trace/TracePlugin.java, lines 168-170 > > <http://review.cloudera.org/r/390/diff/1/?file=3384#file3384line168> > > > > I think you've just re-implemented Arrays.equals(byte[] a, byte[] b). d'oh! > On 2010-07-28 14:25:15, Philip Zeyliger wrote: > > lang/java/src/java/org/apache/avro/ipc/trace/Trace.java, line 88 > > <http://review.cloudera.org/r/390/diff/1/?file=3382#file3382line88> > > > > 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. Changed this to the expression type you list here... wasn't any more or less intuitive to me, but maybe S-expressions will be better for others. - Patrick ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/390/#review521 ----------------------------------------------------------- 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 > >
