You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by Philip Zeyliger <ph...@cloudera.com> on 2010/07/21 22:02:51 UTC
Review Request: AVRO-595
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/351/
-----------------------------------------------------------
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
Re: Review Request: AVRO-595
Posted by Patrick Wendell <pw...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/351/#review490
-----------------------------------------------------------
share/schemas/org/apache/avro/ipc/trace/avroTrace.avdl
<http://review.hbase.org/r/351/#comment2005>
Right not the Transceiver does not expose port information to the Requestor/Responder so this data is hidden. All Transceivers I can imaging will operate over TCP/UDP, so perhaps it makes sense to include a getPort int the Transceiver superclass and then also include port information in the RPCContext that is passed to the plugin?
- Patrick
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
>
>
Re: Review Request: AVRO-595
Posted by Patrick Wendell <pw...@cloudera.com>.
> On 2010-07-21 14:38:28, Philip Zeyliger wrote:
> > lang/java/src/java/org/apache/avro/ipc/trace/TracePlugin.java, lines 176-177
> > <http://review.hbase.org/r/351/diff/1/?file=2993#file2993line176>
> >
> > What's the 100 doing here?
GenericData.Array requires a default capacity. I will probably lower it to 10, but it needs to be specified in constructor.
> On 2010-07-21 14:38:28, Philip Zeyliger wrote:
> > share/schemas/org/apache/avro/ipc/trace/avroTrace.avpr, line 2
> > <http://review.hbase.org/r/351/diff/1/?file=2996#file2996line2>
> >
> > Is this generated? Could this be generated by the build instead of checked in?
Avro IDL is not implemented in other languages, so building this from the avdl file would introduce a dependency on the Java code for building the projects - is that okay?
- Patrick
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/351/#review443
-----------------------------------------------------------
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
>
>
Re: Review Request: AVRO-595
Posted by Philip Zeyliger <ph...@cloudera.com>.
-----------------------------------------------------------
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
>
>