[
https://issues.apache.org/jira/browse/HDFS-14403?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16822104#comment-16822104
]
Erik Krogen commented on HDFS-14403:
------------------------------------
I think I have to agree with [~csun]'s suggestion of splitting this up, as it
is pretty large, and does appear to contain two somewhat unrelated ideas.
[~cgregori] would you mind creating a new JIRA for adding more fine-grained
processing time metrics to the RPC layer?
A few smaller comments:
* I don't really think we should suddenly change the RPC processing time
metrics to micros... I'm worried this might break things that rely on those
metrics. cc [~jojochuang] for an opinion on that. Maybe this needs to be
configurable?
* The {{RpcScheduler}} interface isn't {{@InterfaceAudience.Private}}, so we
should probably maintain compatibility by adding a {{default}} implementation
of the new method which falls back to calling the old one.
* You have a few static imports which appear at the top of the import list, but
they should go at the bottom.
* It seems we don't need to include the {{isResponseDeferred}} argument to
{{updateMetrics}}, since the {{Call}} object is passed in, we can call that
directly. Same for {{timestamp}} and {{processed}}. You also removed a valuable
comment where {{callQueue.take()}} is called.
* The {{methodName}} stuff is a bit messy. At some points in the current patch,
"methodName" will even store the class name of an exception.
* One thing we could do to clean it up a little is, at the start of the two
{{RpcInvoker#call()}} methods, store the value of {{Server.getCurCall()}} and
immediately set the method name. It seems more sensible to me to at least set
the method name at the start, instead of at the very end. Then, we can remove
the {{finally}} blocks entirely, and call {{call.setMethodName()}} again within
the {{catch}} block if necessary. This makes the flow a little cleaner (I
think) but doesn't fix the fact that "methodName" stores other things.
* One other approach I investigated was trying to get the method name within
{{Server#processRpcRequest()}}, directly where the {{RpcCall}} is being
instantiated. We already have some protobuf-specific processing there, so
perhaps we can piggyback off of it. Then, maybe we can reuse
{{RpcCall.responseParams.errorClass}} to differentiate the name used for
processing time within {{updateMetrics}}. We might need to play around with
this a bit.
> Cost-Based RPC FairCallQueue
> ----------------------------
>
> Key: HDFS-14403
> URL: https://issues.apache.org/jira/browse/HDFS-14403
> Project: Hadoop HDFS
> Issue Type: Improvement
> Components: ipc, namenode
> Reporter: Erik Krogen
> Assignee: Christopher Gregorian
> Priority: Major
> Labels: qos, rpc
> Attachments: CostBasedFairCallQueueDesign_v0.pdf,
> HDFS-14403.001.patch, HDFS-14403.002.patch, HDFS-14403.003.patch,
> HDFS-14403.004.patch, HDFS-14403.005.patch, HDFS-14403.branch-2.8.patch
>
>
> HADOOP-15016 initially described extensions to the Hadoop FairCallQueue
> encompassing both cost-based analysis of incoming RPCs, as well as support
> for reservations of RPC capacity for system/platform users. This JIRA intends
> to track the former, as HADOOP-15016 was repurposed to more specifically
> focus on the reservation portion of the work.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org
