[
https://issues.apache.org/jira/browse/HDFS-5274?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13843547#comment-13843547
]
stack commented on HDFS-5274:
-----------------------------
This facility is excellent.
A few notes on the patch.
Long line and formatting seems off here:
{code}
+ if (header.hasTraceInfo() && header.getTraceInfo() != null &&
header.getTraceInfo().hasTraceId()) {
+ String traceDescription = rpcRequest.toString();
+
+ // If the incoming RPC included tracing info, always continue the trace
+ TraceInfo parentSpan = new
TraceInfo(header.getTraceInfo().getTraceId(),
+ header.getTraceInfo().getParentId());
+ traceSpan = Trace.startSpan(traceDescription, parentSpan).detach();
+ }
{code}
This is a javadoc fix?
- * @param buf - SASL wrapped request of one or more RPCs
+ * @param inBuf - SASL wrapped request of one or more RPCs
Are you breaking your pattern of putting trace string building behind a check
that tracing is enabled in the below?
+ TraceScope traceSpan = Trace.startSpan("Call " + method.getName() + " to
" +
+ remoteId, CLIENT_TRACE_SAMPLER);
nit: declare and define all in the one line rather than do initialization in
constructor: + receivers = new HashSet<SpanReceiver>();
Any reason we take config on construction and in init for SpanReceiverHost?
SpanReceiverHost is on only when trace is enabled, right? If so, say so in
class comment.
Has to be a shutdown hook? ShutdownHookManager.get().addShutdownHook ? This is
fine unless we envision someone having to override it which I suppose should
never happen for an optionally enabled, rare, trace function?
HTraceConfiguration is for testing only? Should be @visiblefortesting only or
a comment at least?
nit: Do Classname.simple() here instead? + TraceScope scope =
Trace.startSpan(DFSInputStream.class.getSimpleName());
Should there be defines for a few of these? "DFSInputStream.close" seems
fine... only used once DFSInputStream.read?
Is there something off in the formatting here?
-
- private static class Packet {
- private static final long HEART_BEAT_SEQNO = -1L;
- long seqno; // sequencenumber of buffer in block
- final long offsetInBlock; // offset in block
- boolean syncBlock; // this packet forces the current block to disk
- int numChunks; // number of chunks currently in packet
- final int maxChunks; // max chunks in packet
+ private Span traceSpan = null;
+
+ private static class Packet {
+ private static final long HEART_BEAT_SEQNO = -1L;
+ long seqno; // sequencenumber of buffer in block
+ final long offsetInBlock; // offset in block
+ boolean syncBlock; // this packet forces the current block to disk
+ int numChunks; // number of chunks currently in packet
+ final int maxChunks; // max chunks in packet
Or is it just git cleaning whitespace?
Gratuitous change?
- ClientOperationHeaderProto header =
+ ClientOperationHeaderProto.Builder builder =
ClientOperationHeaderProto.newBuilder()
.setBaseHeader(buildBaseHeader(blk, blockToken))
- .setClientName(client)
- .build();
- return header;
+ .setClientName(client);
+ return builder.build();
Is an annotation of 'J' only intentional?
+ if (traceSpan != null) {
+ traceSpan.addTimelineAnnotation("J");
Put inside a isTracing check? + Trace.addTimelineAnnotation("Connecting
to downstream " + mirrorNode);
ditto: + Trace.addTimelineAnnotation("Waiting for connect ack from
downstream");
> Add Tracing to HDFS
> -------------------
>
> Key: HDFS-5274
> URL: https://issues.apache.org/jira/browse/HDFS-5274
> Project: Hadoop HDFS
> Issue Type: New Feature
> Components: datanode, namenode
> Affects Versions: 2.1.1-beta
> Reporter: Elliott Clark
> Assignee: Elliott Clark
> Attachments: HDFS-5274-0.patch, HDFS-5274-1.patch, HDFS-5274-2.patch,
> HDFS-5274-3.patch, HDFS-5274-4.patch, HDFS-5274-5.patch, HDFS-5274-6.patch,
> Zipkin Trace a06e941b0172ec73.png, Zipkin Trace d0f0d66b8a258a69.png
>
>
> Since Google's Dapper paper has shown the benefits of tracing for a large
> distributed system, it seems like a good time to add tracing to HDFS. HBase
> has added tracing using HTrace. I propose that the same can be done within
> HDFS.
--
This message was sent by Atlassian JIRA
(v6.1.4#6159)
