hadoop-common-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sanjay Radia (JIRA)" <j...@apache.org>
Subject [jira] Commented: (HADOOP-3470) Bad coding style: The member fields in org.apache.hadoop.ipc.metrics.RpcMetrics are public
Date Thu, 11 Dec 2008 19:10:44 GMT

    [ https://issues.apache.org/jira/browse/HADOOP-3470?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12655743#action_12655743

Sanjay Radia commented on HADOOP-3470:

Generally member fields should be accessed via getters and setter. 
In this particular case the purpose of the metrics holder class (such as RPCMetrics or NameNodeMetrics,
etc is to be place holder
of the a list of metrics variables that are then accessed in various parts of the code to
change the counters. 

Forcing a set of new methods per metrics (since a metrics may have multiple operations) is
very painful and will make it 
harder to add new metrics. Adding metrics should be simple"
  - declare metrics variable (eg. counter)
  - write code to change the metrics variable (e.g inc the counter).

(btw HADOOP-4838  simplifies metrics so that only the above two steps are needed.)

I believe the metrics variables are one of the exception to style rule of using getters and
setter for fields.
(Note HADOOP4848 has added a registry and hence it would be easy to have  the callers use
that map but the cost of that
would be too much for changing counters (ie a map lookup operation per counter increment!!)

> Bad coding style: The member fields in org.apache.hadoop.ipc.metrics.RpcMetrics are public
> ------------------------------------------------------------------------------------------
>                 Key: HADOOP-3470
>                 URL: https://issues.apache.org/jira/browse/HADOOP-3470
>             Project: Hadoop Core
>          Issue Type: Improvement
>          Components: metrics
>            Reporter: Tsz Wo (Nicholas), SZE
> In org.apache.hadoop.ipc.metrics.RpcMetrics,
> {code}
> //the following are member fields
>   public MetricsTimeVaryingRate rpcQueueTime = new MetricsTimeVaryingRate("RpcQueueTime");
>   public MetricsTimeVaryingRate rpcProcessingTime = new MetricsTimeVaryingRate("RpcProcessingTime");
>   public Map <String, MetricsTimeVaryingRate> metricsList = Collections.synchronizedMap(new
HashMap<String, MetricsTimeVaryingRate>());
> {code}
> Then, the fields are accessed directly in other classes.  For example, org.apache.hadoop.ipc.RPC.Server.call(...)
> {code}
> ...
> 	MetricsTimeVaryingRate m = rpcMetrics.metricsList.get(call.getMethodName());
> 	if (m != null) {
> 		m.inc(processingTime);
> 	}
> 	else {
> 		rpcMetrics.metricsList.put(call.getMethodName(), new MetricsTimeVaryingRate(call.getMethodName()));
> 		m = rpcMetrics.metricsList.get(call.getMethodName());
> 		m.inc(processingTime);
> 	}
> {code}

This message is automatically generated by JIRA.
You can reply to this email to add a comment to the issue online.

View raw message