[
https://issues.apache.org/jira/browse/HADOOP-16266?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16824359#comment-16824359
]
Erik Krogen commented on HADOOP-16266:
--------------------------------------
Hey [~cgregori], looking good overall! I have some small comments throughout;
see below. Besides those, this could use some unit tests. I still like the idea
of moving to micro or nanoseconds for the processing time, but am still
worried... Ping [~jojochuang] again and also [~elgoiri] for thoughts on whether
we can change the units of these metrics.
* We typically indent parameter lists by 4, rather than to line up with the
first parameter, like:
{code:java}
@Override
public void addResponseTime(String name, Schedulable obj,
ProcessingDetails details) {
}
{code}
rather than
{code:java}
@Override
public void addResponseTime(String name, Schedulable obj,
ProcessingDetails details) {
}
{code}
* You have some extraneous whitespace formatting changes, unless you're making
a change in that line already, it's often better to leave them unchanged (it
makes backports easier).
* I would prefer to avoid the loss of precision by casting to {{int}} in
{{DecayRpcScheduler#addResponseTime()}}. Can we simply throw an
{{UnsupportedOperationException}} within the old version of the method, and
move the implementation to the new one? I don't think the old one will be
called anywhere; we are just leaving it for compatibility with the
{{RpcScheduler}} interface.
* Can we add a {{default}} implementation of the new method within
{{RpcScheduler}}? Otherwise old implementations of this class which may be
floating around will fail. See what I mean
[here|https://dzone.com/articles/interface-default-methods-java]. After this
you shouldn't need to make changes to {{TestRpcScheduler}}.
* For the parameter names {{start}} and {{delta}} within {{FSNamesystemLock}},
can we rename them to {{startNanos}} and {{deltaNanos}} ?
* Does {{ProcessingDetails.Timing.newEnumArray()}} need to be {{public}} ?
Does it even need to exist at all? It seems it's only used in one place.
* Within {{ProtobufRpcEngine}}, we took away the following:
{code:java}
} finally {
currentCallInfo.set(null);
{code}
But I think we should still be resetting the current call info. Also, it looks
like now we only catch and update the name for {{ServiceException}}, but
previously it was for all {{Exception}}, which I think should continue to be
the case. Also in this method, can we store {{Server.getCurCall().get()}}
before the {{try}} block, and then just re-use it? {{ThreadLocal}} is cheap but
not free. (same for {{WritableRpcEngine}})
* {{Server#logSlowRpcCalls()}} should probably use a {{long}} instead of
{{int}} if we are going to use microseconds. Same for the {{RpcMetrics}}
methods.
* Can you add comments describing the various arithmetic ops you do within
{{Server#updateMetrics()}}?
* If we change the {{Call.timestamp}} field from millis to nanos, can we also
rename it to {{timestampNanos}}?
* For {{Call.detailedMetricsName}}, if there are any accesses of
{{getDetailedMetricsName()}} in a different thread from the one which called
{{setDetailedMetricsName()}}, it needs to be {{volatile}}. I'm not sure if this
is the case but it's probably best to do it just in case.
* Can you add at least some simple Javadoc for the new {{Call}} methods?
* It seems a {{LOG}} message was removed from both of the {{RpcEngine}}
classes. A new one was added within {{Server}}, but it is missing information:
the method name, whether it was deferred, and the exception name.
> Add more fine-grained processing time metrics to the RPC layer
> --------------------------------------------------------------
>
> Key: HADOOP-16266
> URL: https://issues.apache.org/jira/browse/HADOOP-16266
> Project: Hadoop Common
> Issue Type: Improvement
> Components: ipc
> Reporter: Christopher Gregorian
> Assignee: Christopher Gregorian
> Priority: Minor
> Labels: rpc
> Attachments: HADOOP-16266.001.patch
>
>
> Splitting off of HDFS-14403 to track the first part: introduces more
> fine-grained measuring of how a call's processing time is split up.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
